-
Notifications
You must be signed in to change notification settings - Fork 22
[misc] upgrade socket.io to version 2.3.0 #117
Conversation
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. |
It looks like the request behaviour is intentional:
https://socket.io/blog/introducing-socket-io-1-0/#New-engine |
From the same page:
every user has a degraded startup experience |
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.
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
|
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have some more comments to follow tomorrow.
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
And the corresponding code in web: https://github.com/overleaf/web-internal/blob/a7bfce35c7b1e1b22fe347d9dd83502e0996f407/frontend/js/ide/editor/Document.js#L518-L540 (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!). |
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. |
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 |
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. |
...and document that we do not emit back to the sender.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good overall 🎉 , just one comment...
[misc] mount socket.io on a configurable path
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.
io.socket.clients(ROOM) -> clientObjectListWithDuplicates
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 thedisconnect
event we can not find out, which rooms the client was in anymore.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.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
andwebsocket
.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