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

fix: Exit on communication failure #429

Merged
merged 5 commits into from
May 10, 2024
Merged
Changes from 1 commit
Commits
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
11 changes: 10 additions & 1 deletion plugins/ui/src/deephaven/ui/object_types/ElementMessageStream.py
Original file line number Diff line number Diff line change
@@ -2,6 +2,8 @@

import io
import json
import sys

from jsonrpc import JSONRPCResponseManager, Dispatcher
import logging
import threading
@@ -352,4 +354,11 @@ def _send_document_update(
logger.debug("Registering callable %s", callable_id)
dispatcher[callable_id] = wrap_callable(callable)
self._dispatcher = dispatcher
self._connection.on_data(payload.encode(), new_objects)
try:
self._connection.on_data(payload.encode(), new_objects)
except RuntimeError as e:
if "io.deephaven.plugin.type.ObjectCommunicationException" in str(e):
# can no longer send, don't need to raise exception but do need to exit
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.

We shouldn't be exiting without at least logging why we're exiting here.
Also I don't like doing a str(e) comparison, is there any more definitive way of checking the exception type?
Include a ticket with steps to reproduce the issue as well so it's clear which scenario is being fixed here. The description in the PR is somewhat vague in the steps.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I have a much better fix now. This is happening after on_close from the MessageStream is called, so I'm now flagging it there. There is still a possibility of throwing if the message stream closes after that check though. Not sure how likely it is. I could add the ugly try catch back (and check against the is_closed variable) too. As far as I can tell there is no better way to catch and check for the specific exception - the error message itself originates in Java and is just put into a generic RuntimeError.
Now that I've refined this, should we be logging? I know we don't want to hide errors, but this is a case where the component should already be gone and not sending anything and a log would just pollute the console.

else:
raise e