[misc] upgrade socket.io to version 2.3.0

[das7pad]

Description

We are finally there: socket.io version two dot three dot zero.

Major API changes

getting clients

The function is async now and has a different signature/return value.

• v0: io.socket.clients(ROOM) -> clientObjectListWithDuplicates
• v2: io.to(ROOM).clients((err, clientIdList) => {}) -> undefined

As we fetch the client asynchronous, some of them may be gone by the time our callback is actually executed. Requiring us to validate each and every client again.
https://github.com/socketio/socket.io-adapter/blob/ae23c7ef4cd1b1793121b5af0376686a2ba2deea/index.js#L210

accessing clients

Clients are stored in an Object now. Available e.g. at io.sockets.connected[SOCKET_ID]

Rooms a client is in

The list at CLIENT.rooms includes the socket id at the first position.

Events

There is a new socket event disconnecting which is emitted just before leaving rooms. From the disconnect event we can not find out, which rooms the client was in anymore.

client.on "disconnect", () ->
    # This is called after leaving rooms.

client.on "disconnecting", () ->
    # This is called just before leaving rooms.

The socket is not tracked anymore by the time disconnect is fired. See this snipped from the DrainManagerTests -- which disconnect all clients -- with additional logging enabled. We no longer have to adjust the number of connected client for the metrics upon disconnecting.

Logging planted into both events respective: ```js # into disconnect console.error('disconnect', Object.keys(io.sockets.connected).length) # into disconnecting console.error('disconnecting', Object.keys(io.sockets.connected).length) ```

  DrainManagerTests
disconnecting 6
disconnect 5
disconnecting 5
disconnect 4
disconnecting 4
disconnect 3
disconnecting 3
disconnect 2
disconnecting 2
disconnect 1
disconnecting 1
disconnect 0
    ✓ should have disconnected all previous clients

---

Other notable changes

middlewares

socket.io v2 supports middlewares, enabling much simpler session handling. The current flow is still functional and this is subject to a followup PR.

cookies

There is a default io cookie emitted now, which we do not need -- embedding the client id in the url continues to work as before.

client side JS serving

The client side JS is served minified by default now.

transports

There are only two transports left: polling and websocket.

---

Related Issues / PRs

https://github.com/overleaf/issues/issues/2757

Review

Some more indent ticks were used to keep git diffs small -- this is again subject to change with the decaffeinate process.

See https://github.com/overleaf/web-internal/pull/2635 / use the jpa-socket-io-v2 branch in web for testing.

Potential Impact

HIGH

Deployment Checklist

See https://github.com/overleaf/issues/issues/2757

Who Needs to Know?

@jdleesmiller

[briangough]

Good work, I've started looking at this.

First question: with socket.io v2 the client connects 3 times to the polling endpoint before connecting to the websocket. Is this expected?

In socket.io v1 it only connects once (directly to the websocket):

[das7pad]

First question: with socket.io v2 the client connects 3 times to the polling endpoint before connecting to the websocket. Is this expected?

I did not find any docs on this. We could list the websocket transport first -- and use polling as fallback. I was not able to change the sequence of the transports in the editor tho. I will experiment with some simpler apps and report back.

[briangough]

It looks like the request behaviour is intentional:

Socket.IO never assumes that WebSocket will just work, because in practice there’s a good chance that it won’t. Instead, it establishes a connection with XHR or JSONP right away, and then attempts to upgrade the connection to WebSocket.

https://socket.io/blog/introducing-socket-io-1-0/#New-engine

[das7pad]

From the same page:

none of your users will have a degraded experience

every user has a degraded startup experience

[das7pad]

I played around in browser stack with IE9 (the first browser I could find that does not support websockets) and the demo twitter stream from the socket.io webpage.

e=document.createElement('script');e.src="https://unpkg.com/socket.io-client@2.3.0/dist/socket.io.js";document.head.appendChild(e)
io.connect('https://socket-io-tweet-stream.now.sh/', {transports: ['websocket','polling'],forceNew:true})

Does not trigger any new connection. With no explicit transports, or a revered sequence, many polling requests are fired. So we definitely want to start with polling then (and the default transport selection).

---

In the socket.io v3 roadmap socketio/socket.io#3250 there is an entry

Default to websocket, and use XHR polling as fallback

[jdleesmiller]

Thanks for looking into it --- I think that behaviour made a lot more sense when socket.io 1 came out than it does now, but in any case I think we can probably live with it, at least initially.

[briangough]

I have some more comments to follow tomorrow.

[briangough]

Another question, socket.io v2 says that it supports binary data, so maybe we can get rid of this encoding code:

real-time/app/coffee/WebsocketController.coffee

Lines 102 to 123 in cb50a91

	# Encode any binary bits of data so it can go via WebSockets
	# See http://ecmanaut.blogspot.co.uk/2006/07/encoding-decoding-utf8-in-javascript.html
	encodeForWebsockets = (text) -> unescape(encodeURIComponent(text))
	escapedLines = []
	for line in lines
	try
	line = encodeForWebsockets(line)
	catch err
	logger.err {err, project_id, doc_id, fromVersion, line, client_id: client.id}, "error encoding line uri component"
	return callback(err)
	escapedLines.push line
	if options.encodeRanges
	try
	for comment in ranges?.comments or []
	comment.op.c = encodeForWebsockets(comment.op.c) if comment.op.c?
	for change in ranges?.changes or []
	change.op.i = encodeForWebsockets(change.op.i) if change.op.i?
	change.op.d = encodeForWebsockets(change.op.d) if change.op.d?
	catch err
	logger.err {err, project_id, doc_id, fromVersion, ranges, client_id: client.id}, "error encoding range uri component"
	return callback(err)

And the corresponding code in web:

https://github.com/overleaf/web-internal/blob/a7bfce35c7b1e1b22fe347d9dd83502e0996f407/frontend/js/ide/editor/Document.js#L518-L540
https://github.com/overleaf/web-internal/blob/a7bfce35c7b1e1b22fe347d9dd83502e0996f407/frontend/js/ide/editor/ShareJsDoc.js#L34-L41

(I actually tried removing that in the current code with socket.io v 0.9 and it seems to work without it -- so I'm not sure if it's even doing anything now!).

[das7pad]

I can write up a PR with binary test data and see how this pans out.

TBH: I would rather keep the scope of the upgrade as small as possible. Let's do these changes separately -- who knows how the upgrade plays with all the different browsers out there.

[briangough]

My other comment - not directly related to this code - is that based on https://socket.io/docs/using-multiple-nodes/ we will need to keep sticky sessions for real-time for the polling fallback to work, so https://github.com/overleaf/google-ops/issues/645 will need to take account of that. cc @henryoswald

[briangough]

I can write up a PR with binary test data and see how this pans out.

TBH: I would rather keep the scope of the upgrade as small as possible. Let's do these changes separately -- who knows how the upgrade plays with all the different browsers out there.

Yes, better to keep this a minimal change. In addition to handling binary data, it looks like we might also be able to make use of the socket.io-redis module for some cleanup in future as well.

[gh2k]

I haven't run this, but from a code perspective (and having done some playing around with socket.io v2 in a hobby project over the last week or so) it looks good. 👍

Will approve once I've had a look at the web end.

[JuneKelly]

Looking good overall 🎉 , just one comment...