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

Improve kernel restart logic to reestablish ZMQ connection when a kernel is restarted with new ports #3284

Closed
wants to merge 4 commits into from
Closed

Improve kernel restart logic to reestablish ZMQ connection when a kernel is restarted with new ports #3284

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9d2bc9a
Add kernel reconnect when kernel is restarted with new ports
skukhtichev Jan 29, 2018
f85e0b7
change log level to debug
skukhtichev Jan 29, 2018
495fc8f
Stream is closed when the kernel is restarted with new ports, Use cre…
skukhtichev Jan 30, 2018
27ade55
Removed empty lines
skukhtichev Jan 31, 2018
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
27 changes: 26 additions & 1 deletion notebook/services/kernels/handlers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,7 +461,32 @@ def _send_status_message(self, status):
msg['channel'] = 'iopub'
self.write_message(json.dumps(msg, default=date_default))

def on_kernel_restarted(self):
def on_kernel_restarted(self, **kwargs):
if 'newports' in kwargs:
self.log.debug("Kernel restarted with new ports")
newports = kwargs['newports']
else:
self.log.debug('newports parameter is not defined, setting default to False')
newports = False
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be simplified:

def on_kernel_restarted(self, newports=False):
   ...

logging.info("Restarting kernel with new ports: {}".format(newports))
if newports:
# Stream should be closed before reconnecting to the new ports
for channel, stream in self.channels.items():
if stream is not None and not stream.closed():
stream.on_recv(None)
stream.close()
# Connect to the stream with new ports
try:
self.create_stream()
except web.HTTPError as e:
Copy link
Member

Choose a reason for hiding this comment

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

I can see this is copied from the open() method, but I don't think create_stream can actually raise HTTPError. Maybe it could at some point in the past. @minrk any ideas?

Copy link
Member

Choose a reason for hiding this comment

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

I think you're right. My guess is there used to be code in there where we were retrieving the kernel, which could raise 404, etc. Or it was just a mistake and this should have been Exception all along, which is what it should be now.

self.log.error("Error opening stream after kernel restarted: %s", e)
for channel, stream in self.channels.items():
if not stream.closed():
stream.close()
self.close()
return
for channel, stream in self.channels.items():
stream.on_recv_stream(self._on_zmq_reply)
logging.warn("kernel %s restarted", self.kernel_id)
self._send_status_message('restarting')

Expand Down