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

Disable http fallback after successful websocket connection #10395

Merged
merged 1 commit into from
Nov 12, 2021

Conversation

msujew
Copy link
Member

@msujew msujew commented Nov 8, 2021

What it does

Closes #10261

Simply disables the fallback by setting the allowed property on the fallback options to false once a websocket connection has been established.

How to test

  1. Start Theia (Browser) on a remote server
  2. Open the Theia window in your browser
  3. Disconnect from the remote server (unplug your ethernet)
  4. The http fallback should not activate
  5. After re-establishing network connection, Theia should re-establish its connection as well.

Review checklist

Reminder for reviewers

@msujew msujew added the messaging issues related to messaging label Nov 8, 2021
@paul-marechal
Copy link
Member

I feel like this HTTP fallback depends on where/how a Theia instance is deployed, and as such I'm wondering if it's a good idea to always fallback to it? Assuming a WebSocket connection is supposed to work, is it a good idea to try to use HTTP if that fails?

I'm thinking that we could have an option like connectionType: 'single-websocket' | 'http-long-polling' to have Theia explicitly operate in one mode or the other?

@msujew
Copy link
Member Author

msujew commented Nov 9, 2021

I feel like this HTTP fallback depends on where/how a Theia instance is deployed

@paul-marechal Well yes, but also no. It depends more on the network configuration of the user. The main use case for the http fallback is for users in heavily firewalled corporate environments where websockets are blocked. The ones responsible for the deployment don't really know, whether their users will be restricted to http only.

I'm thinking that we could have an option like connectionType: 'single-websocket' | 'http-long-polling' to have Theia explicitly operate in one mode or the other?

I'm not really in favor of that, since the long polling option is considerably slower in a lot of cases, where a lot of data is send between the clients.

Instead, I would change the default implementation to prevent using the http fallback solution. Downstream users would have to explicitly enable the option. WDYT?

@paul-marechal
Copy link
Member

I understand now, you are right. We can keep the HTTP fallback and forget about making it configurable.

Copy link
Member

@paul-marechal paul-marechal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM

@paul-marechal
Copy link
Member

paul-marechal commented Nov 9, 2021

But this got me thinking about what we do with this WebSocket multiplexing and HTTP fallback, is there any advantage to doing it ourselves over using https://socket.io/ ?

socket.io supports multiplexing, reconnection, and HTTP fallback.

@msujew
Copy link
Member Author

msujew commented Nov 10, 2021

is there any advantage to doing it ourselves over using https://socket.io/ ?

There isn't, I just didn't know about socket.io so far. I will create an issue for that and try integrating it into Theia when I have some time for that.

@msujew msujew merged commit c558358 into master Nov 12, 2021
@msujew msujew deleted the msujew/disable-http-fallback branch November 12, 2021 18:22
@github-actions github-actions bot added this to the 1.20.0 milestone Nov 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
messaging issues related to messaging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Automatic websocket reconnection after transient offline events is broken
2 participants