fix: add reconnect support to JsWidget so deephaven.ui widgets revive#8239
fix: add reconnect support to JsWidget so deephaven.ui widgets revive#8239fyrkov wants to merge 3 commits into
Conversation
No docs changes detected for e17ea57 |
niloc132
left a comment
niloc132
left a comment
There was a problem hiding this comment.
Broadly, reconnect isn't possible for streams where state in the stream matters - any stream where messages build on each other rather than sending full snapshots of current state will prevent the client from having an accurate picture. Those cases need to be fully aware that the stream was lost, messages were lost, and the stream with all of its state needs to be recreated from scratch.
This isn't to say that what you're trying to achieve isn't possible, just that it is critical to go after the specifics, and the right answer is probably somewhere between the current implementation and "just fire more message events". Existing consumers of plugins that expect messages to stop after disconnect might have undefined behavior now where they stop listening to messages, and are surprised to find that a new stream was created on their behalf, leaking export tickets sent server->client along the way...
To be fair, the current state of affairs isn't very nice, but we did paint ourselves into a corner by not contemplating all bidi plugin use cases from the very first revision, and then started producing plugins that stuck to those assumptions. Adding more features in backwards compatible ways has been difficult, but there are 3rd party plugins in the wild now, and we need to be wary of breaking them in silent ways.
My comments below mostly are about the re-export piece, which I think is unrelated to the "reconnect the stream" issue that the issue and PR are about. If we can split that off to review/discuss separately, I'd like to explore just the stream reconnection issue and potential solutions here. If I've misunderstood details about this approach, please let me know.
Agreed - removed entirely. This PR is now just same-session stream reconnect, matching the issue. |
8d2a823 to
9d1178e
Compare
- JsWidget.die() now unregisters from simpleReconnectableInstances (mirroring close()), so a widget whose revive fails stops receiving disconnect/refetch callbacks on every future reconnect. - getWidget(JsVariableDefinition) delegates to getWidget(TypedTicket) instead of duplicating its construction body.
2 participants
JsWidget extended HasEventHandling and was never registered as reconnectable, so after a disconnect its message stream was nulled and never reopened - every subsequent message became a silent no-op, leaving deephaven.ui components (e.g. a ui.button) dead until page reload.
Make JsWidget lifecycle-aware (extend HasLifecycle) and register plain widgets as reconnectable, so on reconnect it reopens its stream, emits disconnect/ reconnect events, and re-delivers the current document.
Fixes #8240