Add ping to WebSocket

[dbasedow]

Idle WebSocket connections get reset after a few minutes of inactivity. To prevent this, most WebSocket implementations offer sending special ping messages. This PR adds a method sendPing() to WebSocket. Ping payloads are not supported.

Manual testing can be done by adding connection.on('ping', _ => console.log('Received ping')); to a ws connection or using a packet sniffer while sending pings.

[ghost]

By analyzing the blame information on this pull request, we identified @philikon and @nicklockwood to be potential reviewers.

[satya164]

This is not part of the WebSocket spec, right? How do other libs implement it?

[dbasedow]

It's not part of the JS browser API. Most low level WebSocket implementations expose a ping or sendPing method. Usually you can add data to the ping that will be sent back as a pong by the server.

Higher level libs like socket.io do not directly expose a method for pinging but instead manages the regular pinging internally: https://github.com/socketio/engine.io-client/blob/2c55b278a491bf45313ecc0825cf800e2f7ff5c1/lib/socket.js#L487

[satya164]

It looks like something which can be implemented on JS side on top of existing APIs. I think we don't need to have this in core. Let me know if I'm wrong.

[dbasedow]

It's not possible to do this in JS. WebSocket pings are control frames with a different op-code than messages (see rfc). The underlying libraries already contain methods for ping, close and pong, the three control frames currently implemented, but only close is exposed in JS.

You could of course send regular messages to the server to keep the connection open, but ping frames are tailor made for this. Regular messages would potentially require changes in the server component to handle these messages. Ping control frames are handled by all WebSocket implementations I know of. For me this is not really an alternative.

If you really don't want this in core, maybe we could make the Android implementation of WebSocket extendable? For iOS we already released an app with ping capability, with no changes to core. I put a category on RCTWebSocketModule adding the sendPing method and extended the JS WebSocket module, adding sendPing there as well.

With Android I would have to use reflection or copy the existing implementation since mWebSocketConnections is private and there's no getter. Both options are not ideal. Adding a getWebSocketConnection method would help, but this would tie the extension to the underlying WebSocket implementation which is also bad.

[satya164]

It's not possible to do this in JS. WebSocket pings are control frames with a different op-code than messages (see rfc). The underlying libraries already contain methods for ping, close and pong, the three control frames currently implemented, but only close is exposed in JS.

I see. Thanks for the link.

If you really don't want this in core, maybe we could make the Android implementation of WebSocket extendable? For iOS we already released an app with ping capability, with no changes to core. I put a category on RCTWebSocketModule adding the sendPing method and extended the JS WebSocket module, adding sendPing there as well.

I've nothing against adding it to core. Was just checking if it's possible without adding an API.

Since it's a control frame like close, let's rename it to ping instead of sendPing for consistency. Also, why not expose pong also? Reading the RFC, I see that the server can also send pings and the client should respond with pongs.

cc @dmmiller @astreet

[philikon]

Reading the RFC, I see that the server can also send pings and the client should respond with pongs.

It seems that RCTSRWebSocket already handles that automatically: https://github.com/facebook/react-native/blob/master/Libraries/WebSocket/RCTSRWebSocket.m#L712-L720. okhttp3 does as well: https://github.com/square/okhttp/blob/master/okhttp-ws/src/main/java/okhttp3/internal/ws/RealWebSocket.java#L65-L74

As they should IMHO. The ping/pong messages seem like an implementation detail of the protocol. I'm not 100% sure we should even expose sending pings, rather than doing it automatically for the user (which is what browsers seem to do), but adding it can't hurt for now. I agree that we should call it ping() rather than sendPing() to mirror node's ws module.

[dmmiller]

Agree with @satya164 and @philikon, let's call it ping and then let's ship it. We can look later if we want to move it internally and make the native code handle it on a timer.

[satya164]

@dbasedow Let's rename the native methods as well.

[dbasedow]

I changed the name to ping in the native modules as well.

[dmmiller]

@facebook-github-bot shipit

[ghost]

Thanks for importing. If you are an FB employee go to Phabricator to review.

[philikon]

FWIW, unless I'm misreading the Firefox source, it seems that at least Firefox isn't sending pings by default. (And I'm not even sure it's sending them on a regular basis.) I couldn't be bothered to test this, though ;). Anyway, that makes a strong case for providing the API so that consumers can do something with ping if they want to.

[wprater]

curious why the pong response is not exposed as an event to the bridge? should we consider adding this?

/cc @dbasedow