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

Support transparent re-connects on the server side #18

Closed
DanTup opened this issue Dec 16, 2019 · 2 comments · Fixed by #19
Closed

Support transparent re-connects on the server side #18

DanTup opened this issue Dec 16, 2019 · 2 comments · Fixed by #19

Comments

@DanTup
Copy link
Contributor

DanTup commented Dec 16, 2019

#17 added retry logic to the client, but as far as the server sees this is a new SseConnection client connection and the old SseConnection stream completes.

In dwds, the connection closing triggers the services shutting down:

https://github.com/dart-lang/webdev/blob/2958ede70f20c402e869b4169f3d2f97162e51d4/dwds/lib/src/handlers/dev_handler.dart#L186-L194

It would be nice if the server was able to handle reconnects (within some time period, and opted-in) transparently so the connection never closes and the streams on SseConnection are automatically connected/proxied to the new connection.

DanTup added a commit to DanTup/sse that referenced this issue Dec 17, 2019
@grouma grouma closed this as completed in #19 Jan 6, 2020
grouma pushed a commit that referenced this issue Jan 6, 2020
@grouma this is an attempt to fix #18 (may be easier to [view the diff ignoring whitespace](https://github.com/dart-lang/sse/pull/19/files?utf8=%E2%9C%93&diff=unified&w=1) since some code got indenting and makes the diff look much bigger than it is).

However there is an exposed method here - `closeSink` that closes the underlying sink (in order to test - we can't close the exposed `sink` because it closes the stream controller that needs to continue to be used). I'm not sure if there's a better way (we can add `@visibleForTesting`, though `meta` isn't currently referenced here).

Happy to make changes if this isn't what you had in mind (and I can test it end-to-end through dwds and GitPod to confirm it works prior to merging it).
@DanTup
Copy link
Contributor Author

DanTup commented Jan 24, 2020

@grouma thanks for merging and releasing! I'd like to update dwds with this - do you have a preference for how it's used - should it be passed in (eg. by Flutter) and default to null (current behaviour), or should dwds default to some timeout (a change in behaviour, but maybe a good one)? If the latter, should the timeout be overridable (eg. by Flutter) and what would you be happy with as a default timeout?

When testing on GitPod, I was using 30s timeouts in these locations:

@grouma
Copy link
Member

grouma commented Jan 24, 2020

I don't see any harm just setting a default value in DWDS itself. I don't see the need for making the value configurable.

DanTup added a commit to DanTup/webdev that referenced this issue Jan 27, 2020
This allows the server to accept client reconnections and present them transparently to the consuming code (see dart-lang/sse#18).
DanTup added a commit to DanTup/webdev that referenced this issue Jan 31, 2020
This allows the server to accept client reconnections and present them transparently to the consuming code (see dart-lang/sse#18).
grouma pushed a commit to dart-lang/webdev that referenced this issue Feb 13, 2020
* Bump sse to v3.1.1 and add server keepalive

This allows the server to accept client reconnections and present them transparently to the consuming code (see dart-lang/sse#18).

* Update CHANGELOG.md

* Disconnect any old isolate
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 a pull request may close this issue.

2 participants