Abort queued messages if an error occurs #264
Conversation
| c(sockets$shell), # only shell channel | ||
| c(.pbd_env$ZMQ.PO$POLLIN), # type | ||
| 0) # no timeout, only what's there | ||
| if (ret != 1){ |
There was a problem hiding this comment.
Not sure if that't the correct way to break here, but for my test case, it was always 1 for the other requests and 0 afterwards.
There was a problem hiding this comment.
According to the docs, zmq.poll should return two values: 'a ZMQ code and an errno'. So I suspect comparing it to a single integer is not the right thing to do. I'd be inclined to do it as an else clause of the bit below, that way there's only one thing we're checking instead of two.
There was a problem hiding this comment.
I read that too, the 1 vs 0 is from print(ret)... :-/ The main problem I have with the else in the next hunk is, that I actually have no idea what this check does :-/
There was a problem hiding this comment.
The check below is, unless I'm misunderstanding it, the standard 'are there any messages ready on the shell socket' - the shell socket being input number 1 (silly 1-based indexing) to zmq.poll above.
zmq.poll.get.revents(1)-> which events did we get for socket number 1.pbd_env$ZMQ.PO$POLLIN-> The 'messages to read' eventbitwAnd-> do the events include POLLIN?
There was a problem hiding this comment.
Ok, added the new check...
| @@ -129,6 +129,35 @@ handle_shell = function() { | |||
| print(c('Got unhandled msg_type:', msg$header$msg_type))) | |||
| }, | |||
|
|
|||
| handle_abort = function(){ | |||
There was a problem hiding this comment.
Can we call this something like abort_shell_msg? handle_x usually means that there is an x arriving that we need to deal with.
The notebook send all execution requests without waiting for the reply and expects the kernel to abort requests if an error occurs (that's at least the behavior of the python kernel). After this commit, the R kernel aborts all shell messages (where execution requests come in) when the kernel was either interrupted or an error occurred during the execution of some code. Fixes: IRkernel#232
|
The new check for "empty queue" was actually fixing a bug: it seems that in some cases the first check for the return code wasn't terminating and therefore any new code cell wasn't executed either... At least I never got a result for the next cell after a library failed to load. @takluyver Such things should be added to the kernel tester:
|
|
Thanks, I think this looks good. And yeah, go ahead and make an issue on jupyter_kernel_test for abort behaviour. |
Abort queued messages if an error occurs
| @@ -258,7 +259,13 @@ execute = function(request) { | |||
| } | |||
|
|
|||
| send_response('execute_reply', request, 'shell', reply_content) | |||
|
|
|||
|
|
|||
| if (interrupted || !is.null(err$ename)){ | |||
There was a problem hiding this comment.
space between ) { (also below 2 times)
The notebook send all execution requests without waiting for the reply and
expects the kernel to abort requests if an error occurs (that's at least the
behavior of the python kernel).
After this commit, the R kernel aborts all shell messages (where execution
requests come in) when the kernel was either interrupted or an error occurred
during the execution of some code.
Fixes: #232