Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make defaults for socket.io transports consistent to use polling before websocket #202

Merged
merged 1 commit into from Mar 19, 2016

Conversation

xPaw
Copy link
Member

@xPaw xPaw commented Mar 17, 2016

Since we chose to support modern browsers, we should be just fine defaulting to websockets, if the browser does not support them, it will fallback to http polling.

Code already defaults to this order here: https://github.com/thelounge/lounge/blob/f577da52a39d118c9f6256101a7cd3828dcfc67d/src/server.js#L28

@xPaw xPaw added the Type: Feature Tickets that describe a desired feature or PRs that add them to the project. label Mar 17, 2016
@AlMcKinlay
Copy link
Member

So, as mentioned in the IRC, I don't think we should do this. The reason it does this is so that if you can't establish websockets because of network issues, you are still connected from straight away. If you don't, you can have up to 30 seconds without a connection to the server.

@astorije
Copy link
Member

Just for the record, here is the IRC discussion you guys had (I had to dig up my 600+ unread messages, thanks for mentioning you discussed about it, @YaManicKill :-) ):

Ndushi: when you use the remember-me token to sign in, all socket.io communication seems to go via polling instead of websocket
Ndushi: this cannot be intended right?
Ndushi: can someone else verify this?
Ndushi: oh it seems to be just the auth request and initial response :S
Max-P: Yeah socket.io does weird things
prawnsalad: socket.io really does suck once you get passed 2 users
YaManicKill: Socket.io doesn't suck necessarily. I'm using it on a site with 10k users and never had any issues since 1.0
xPaw: socket.io uses polling by default I think, however, since we only support modern browsers, do we need it?
Ndushi: polling or socket.io?
xPaw: socket.io uses polling
xPaw: then upgrades to websockets
Ndushi: yeah, I meant: what do we not need - polling or socket.io?
xPaw: Ndushi, polling is part of socket.io...
Ndushi: yeah, ok you meant just disable it
xPaw: no I meant change order, see pull requets
Ndushi: everything cleared up when I read the PR :P
YaManicKill: xPaw: yeah, that's so that there isn't an issue if networks don't support websockets
YaManicKill: So that you always have a connection.,
YaManicKill: It's something they upgraded to in 1.0
xPaw: in our case fallback is better, imo
YaManicKill: I disagree. If someone can't connect to websockets, they can have up to 30 seconds without any connection before it falls back to polling.
xPaw: if we dont change it, we need to change the default in code and add a faq entry for it
Ndushi: could you inspect it and check it out?
YaManicKill: xPaw: why? Why do we need to change it? It's not causing any problems.
xPaw: YaManicKill, change the default in code I mean
YaManicKill: Ah right, gotcha.

Also, another FYI, this feature/setting was added by erming/shout#267.

First, I agree that whatever the choice is, defaults and fallbacks should match.

That being said, there are a few key questions to answer here:

  • What are the effects of using polling-first? Can that prevent to use websocket in some configurations? When does one get used vs the other in this order?
  • What are the consequences of using websocket-first? Can that entirely break in some configurations? Does that gracefully degrade? Are there advantages to do so?
  • ... Do we need such configuration altogether? If targeting modern systems (web servers, Node versions, browser versions, ...), does that even make sense to fallback to polling? Would that provide any advantages to such modern environments to remove it?
  • Whether removing polling or setting/changing transport order, what systems are affected? Specifically, which versions of Nginx, Apache, Node.js, Chrome, Firefox, IE/Edge, Android, iOS (others in a reasonable way?) do and do not support websockets? How would they be affected by removing polling or setting a specific order: entirely broken, slower in general, slower on every first connection, ... ?

@AlMcKinlay
Copy link
Member

What are the effects of using polling-first? Can that prevent to use websocket in some configurations? When does one get used vs the other in this order?

No, polling doesn't stop websockets happening, it just means that the first few requests are a bit slower than if they were over websockets. But because websockets takes more setup, they still happen quicker, and once websockets is set-up, it switches over to that

What are the consequences of using websocket-first? Can that entirely break in some configurations? Does that gracefully degrade? Are there advantages to do so?

There is 1 case that this can fail completely, which I have come across before.

If websockets are blocked on the network level (on specific port or just entirely) then it can take up to 30 seconds for socket.io to realise that you haven't connected, and try polling instead/

... Do we need such configuration altogether? If targeting modern systems (web servers, Node versions, browser versions, ...), does that even make sense to fallback to polling? Would that provide any advantages to such modern environments to remove it?

It's not about browsers, it's about the networks. You can be on the latest everything, but at work websockets are blocked (it has happened on a project I worked on) then you have a long time without any connection, and it can seem like it's entirely broken.

Setting up polling first means that everyone is connected from the get-go. Then socket.io immediatelly tries to set up websockets, and if it connects, then it turns off polling and switches to the websocket connection. I see no reason to switch this around, as it doesn't slow anything down, and can massively speed it up for some people.

@astorije
Copy link
Member

Thanks a lot for this explanation, @YaManicKill, it makes complete sense.

@xPaw, unless there is something I misunderstood, I think I agree with @YaManicKill here. What do you think of having consistent defaults and fallbacks?

@xPaw
Copy link
Member Author

xPaw commented Mar 19, 2016

@astorije I will rebase the PR later to change the fallback to match default config.

@xPaw xPaw self-assigned this Mar 19, 2016
@xPaw
Copy link
Member Author

xPaw commented Mar 19, 2016

Rebased.

@AlMcKinlay
Copy link
Member

👍

@astorije
Copy link
Member

👍 and merging. Thanks!

astorije added a commit that referenced this pull request Mar 19, 2016
Change default socket.io transports to use websocket over polling
@astorije astorije merged commit 00f071d into master Mar 19, 2016
@astorije astorije deleted the xpaw/ws branch March 19, 2016 17:26
@astorije astorije changed the title Change default socket.io transports to use websocket over polling Make defaults for socket.io transports consistent to use polling before websocket Mar 20, 2016
@astorije astorije added this to the 1.4.0 milestone Oct 8, 2016
@xPaw xPaw removed their assignment Mar 12, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Tickets that describe a desired feature or PRs that add them to the project.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants