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

Add basic websockets support #1027

Merged
merged 9 commits into from
Oct 16, 2024
Merged

Add basic websockets support #1027

merged 9 commits into from
Oct 16, 2024

Conversation

wwwillchen
Copy link
Collaborator

@wwwillchen wwwillchen commented Oct 13, 2024

Summary of changes:

  • Adds basic websocket support on both the server and client side.
  • The initial render will use a regular SSE HTTP connection because it needs to fetch the experiment flag. After that, if websockets is enabled (i.e. MESOP_WEBSOCKETS_ENABLED env var is set to true), then all the subsequent UI requests and responses will happen on a single websocket connection.
  • This partially addresses the use case in Support WebSockets (as an alternative to SSE) #1028. One thing that's missing is that different websocket messages are handled by separate Mesop Context instances, which means that in the concurrent request use case, you can still hit the bug detailed in that issue. I've explored ways of addressing this (e.g. reusing the same Mesop context instance for the same websockect connection), but there are some non-trivial edge cases (e.g. how do we avoid data race conditions in mutating the same component tree) that I've deferred it for a future PR.

Notes:

  • The security issues found by Snyk for werkzeug aren't actually introduced in this PR and they only affect Mesop framework developers.
  • Remove unused _requests_in_flight code.
  • Updated a couple e2e tests that were implicitly relying on the blur getting fired first to explicitly trigger blur. When I tested it manually the blur event almost always happens first.
  • I'll try to sync this downstream (looks like socket-io.client is available downstream)

@wwwillchen wwwillchen changed the title Prototype websockets Add websockets support Oct 15, 2024
@wwwillchen wwwillchen changed the title Add websockets support Add basic websockets support Oct 15, 2024
@wwwillchen wwwillchen marked this pull request as ready for review October 15, 2024 03:47
Copy link
Collaborator

@richard-to richard-to left a comment

Choose a reason for hiding this comment

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

Awesome. Nice that the the overall amount of non-boilerplate changes was pretty minimal from SSE to WebSocket.

Have you tested on Cloud Run?

I tested it out a bit, mostly with the animation demo and it worked pretty well. I didn't run into the same issues as #981.

It still has the issue of #980 but I think that's resolvable by not deleting the sessions objects right away. Not sure if that's a good long term approach. But I can try to prioritize that fix.

Been a while since I've used socket io, but does socket io still have fallbacks when web sockets are not possible --- I'm assuming not since back in the day web socket support was not in all browsers yet?

And thanks for offering to handle the downstream sync. 🙇🏾 🙇🏾 🙇🏾

.github/workflows/ci.yml Outdated Show resolved Hide resolved
mesop/web/src/services/channel.ts Show resolved Hide resolved
mesop/web/src/services/channel.ts Show resolved Hide resolved
@wwwillchen
Copy link
Collaborator Author

wwwillchen commented Oct 16, 2024

Awesome. Nice that the the overall amount of non-boilerplate changes was pretty minimal from SSE to WebSocket.

Agreed :) our server/channel abstraction ain't too bad :D

Have you tested on Cloud Run?

I haven't but I'll try to test it out sometime. Theoretically it should work because Cloud Run supports websockets, but I'm not sure if the cutover will work (e.g. a request gets routed to another Cloud Run instance).

I tested it out a bit, mostly with the animation demo and it worked pretty well. I didn't run into the same issues as #981.

It still has the issue of #980 but I think that's resolvable by not deleting the sessions objects right away. Not sure if that's a good long term approach. But I can try to prioritize that fix.

Been a while since I've used socket io, but does socket io still have fallbacks when web sockets are not possible --- I'm assuming not since back in the day web socket support was not in all browsers yet?

Yeah it still has fallback support with HTTP long-polling. We could support making this configurable, although, I think for Mesop, it's probably better to just use the default SSE approach.

And thanks for offering to handle the downstream sync. 🙇🏾 🙇🏾 🙇🏾

@wwwillchen wwwillchen merged commit 4a48fec into google:main Oct 16, 2024
5 of 6 checks passed
@wwwillchen wwwillchen deleted the websockets branch October 16, 2024 07:04
@richard-to
Copy link
Collaborator

Yeah it still has fallback support with HTTP long-polling. We could support making this configurable, although, I think for Mesop, it's probably better to just use the default SSE approach.

I see. Yeah I was thinking the long polling probably wouldn't be a great experience (compared to SSE). Would there be a way to disable that fallback?

In terms of options from there, I guess we could try to fallback to the Mesop SSE in that scenario. But I do wonder if the behaviors/functionality would diverge too much after a while. So if someone enabled Websockets, then they probably are expecting certain functionality potentially. So the other option is to just sort of fail for those users, but that's also not a great experience (but I imagine web socket support is pretty common nowadays so seems like an uncommon scenario.

@wwwillchen
Copy link
Collaborator Author

Yeah it still has fallback support with HTTP long-polling. We could support making this configurable, although, I think for Mesop, it's probably better to just use the default SSE approach.

I see. Yeah I was thinking the long polling probably wouldn't be a great experience (compared to SSE). Would there be a way to disable that fallback?

It should be disabled:

transports: ['websocket'],

In terms of options from there, I guess we could try to fallback to the Mesop SSE in that scenario. But I do wonder if the behaviors/functionality would diverge too much after a while. So if someone enabled Websockets, then they probably are expecting certain functionality potentially. So the other option is to just sort of fail for those users, but that's also not a great experience (but I imagine web socket support is pretty common nowadays so seems like an uncommon scenario.

Yeah I think failing is OK. My guess is websockets will be used only in advanced use cases, where the developer should have a good idea of whether its supported.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants