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.

Sign up for GitHub

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

fix: Exit on communication failure #429

Merged
merged 5 commits into from
May 10, 2024
Merged

fix: Exit on communication failure #429

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
60d15bb
exit on communication failure
jnumainville Apr 19, 2024
925dcd3
exit on close
jnumainville Apr 22, 2024
beb78d5
Merge remote-tracking branch 'origin/main' into catch_on_data
jnumainville May 1, 2024
5e2e01b
comments
jnumainville May 3, 2024
6dbb793
Merge remote-tracking branch 'origin/main' into catch_on_data
jnumainville May 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import io
import json
import sys

from jsonrpc import JSONRPCResponseManager, Dispatcher
import logging
import threading
Expand Down Expand Up @@ -119,6 +121,11 @@ class ElementMessageStream(MessageStream):
Captured ExecutionContext for this stream, to wrap all user code.
"""

_is_closed: bool
"""
Whether or not the stream is closed. If closed, no more messages can be sent, and this component should exit.
"""

def __init__(self, element: Element, connection: MessageStream):
"""
Create a new ElementMessageStream. Renders the element in a render context, and sends the rendered result to the
Expand All @@ -142,6 +149,7 @@ def __init__(self, element: Element, connection: MessageStream):
self._is_dirty = False
self._render_state = _RenderState.IDLE
self._exec_context = get_exec_ctx()
self._is_closed = False

def _render(self) -> None:
logger.debug("ElementMessageStream._render")
Expand Down Expand Up @@ -240,7 +248,7 @@ def start(self) -> None:
self._connection.on_data(b"", [])

def on_close(self) -> None:
pass
self._is_closed = True
jnumainville marked this conversation as resolved.
Show resolved Hide resolved

def on_data(self, payload: bytes, references: list[Any]) -> None:
"""
Expand Down Expand Up @@ -352,4 +360,6 @@ def _send_document_update(
logger.debug("Registering callable %s", callable_id)
dispatcher[callable_id] = wrap_callable(callable)
self._dispatcher = dispatcher
if self._is_closed:
sys.exit()
Copy link
Member

Choose a reason for hiding this comment

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

Just to be clear - this is just a safeguard for a race condition if the connection closes while it's rendering, yeah? In which case add a comment to that effect. Do we need the sys.exit() and does that just exit this thread or ?

Copy link
Collaborator Author

@jnumainville jnumainville Apr 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

The sys.exit() closes this thread only. The sys.exit() (or at least something that closes this thread) is necessary because as of right now, the thrown exception is what shuts down this thread. If it isn't caught, the thread will keep trying to send updates. Really, sys.exit() just throws an error itself, but a silent one.

self._connection.on_data(payload.encode(), new_objects)
Loading