From 45afaab2d0779455b1bc9fbd2cc216af8bc21abc Mon Sep 17 00:00:00 2001 From: Glen Takahashi Date: Mon, 14 Dec 2020 13:23:47 -0800 Subject: [PATCH 1/6] Revert "Revert "Fix stop_on_error_timeout blocking other messages in queue"" This reverts commit dc8461c3dee2132fca3d789e685bf8700ec742b9. --- ipykernel/kernelbase.py | 14 ++++---------- 1 file changed, 4 insertions(+), 10 deletions(-) diff --git a/ipykernel/kernelbase.py b/ipykernel/kernelbase.py index 5a988d37e..af051067b 100644 --- a/ipykernel/kernelbase.py +++ b/ipykernel/kernelbase.py @@ -40,7 +40,6 @@ CONTROL_PRIORITY = 1 SHELL_PRIORITY = 10 -ABORT_PRIORITY = 20 class Kernel(SingletonConfigurable): @@ -792,16 +791,11 @@ def _abort_queues(self): stream.flush() self._aborting = True - self.schedule_dispatch( - ABORT_PRIORITY, - self._dispatch_abort, - ) + def stop_aborting(f): + self.log.info("Finishing abort") + self._aborting = False - @gen.coroutine - def _dispatch_abort(self): - self.log.info("Finishing abort") - yield gen.sleep(self.stop_on_error_timeout) - self._aborting = False + self.io_loop.add_future(gen.sleep(self.stop_on_error_timeout), stop_aborting) def _send_abort_reply(self, stream, msg, idents): """Send a reply to an aborted request""" From 9655cfdc52d85dfffb38c48b806679f4b14e69aa Mon Sep 17 00:00:00 2001 From: Glen Takahashi Date: Mon, 14 Dec 2020 14:16:25 -0800 Subject: [PATCH 2/6] Only abort execute_requests --- ipykernel/kernelbase.py | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/ipykernel/kernelbase.py b/ipykernel/kernelbase.py index af051067b..9c272c5e9 100644 --- a/ipykernel/kernelbase.py +++ b/ipykernel/kernelbase.py @@ -181,10 +181,6 @@ def dispatch_control(self, msg): # Set the parent message for side effects. self.set_parent(idents, msg) self._publish_status('busy') - if self._aborting: - self._send_abort_reply(self.control_stream, msg, idents) - self._publish_status('idle') - return header = msg['header'] msg_type = header['msg_type'] @@ -232,7 +228,10 @@ def dispatch_shell(self, stream, msg): self.set_parent(idents, msg) self._publish_status('busy') - if self._aborting: + msg_type = msg['header']['msg_type'] + + # Only abort execute requests + if self._aborting and msg_type == 'execute_request': self._send_abort_reply(stream, msg, idents) self._publish_status('idle') # flush to ensure reply is sent before @@ -240,8 +239,6 @@ def dispatch_shell(self, stream, msg): stream.flush(zmq.POLLOUT) return - msg_type = msg['header']['msg_type'] - # Print some info about this message and leave a '--->' marker, so it's # easier to trace visually the message chain when debugging. Each # handler prints its message at the end. From 297e06c7eb00a83eaf24681f5a881850828c1df9 Mon Sep 17 00:00:00 2001 From: Glen Takahashi Date: Mon, 14 Dec 2020 15:38:43 -0800 Subject: [PATCH 3/6] Add test --- ipykernel/tests/test_kernel.py | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/ipykernel/tests/test_kernel.py b/ipykernel/tests/test_kernel.py index 8cfcd3b2a..9602dd737 100644 --- a/ipykernel/tests/test_kernel.py +++ b/ipykernel/tests/test_kernel.py @@ -391,3 +391,29 @@ def test_interrupt_during_pdb_set_trace(): validate_message(reply, 'execute_reply', msg_id) reply = kc.get_shell_msg(timeout=TIMEOUT) validate_message(reply, 'execute_reply', msg_id2) + +def test_abort_execute_requests(): + """test that execute_request's are aborted after an error""" + with kernel() as kc: + msg_id1 = kc.execute(code="assert False") + msg_id2 = kc.execute(code="assert True") + reply1 = kc.get_shell_msg(timeout=TIMEOUT) + reply2 = kc.get_shell_msg(timeout=TIMEOUT) + assert reply1['content']['status'] == 'error' + assert reply2['content']['status'] == 'aborted' + +def test_dont_abort_non_execute_requests(): + """test that kernel_info, comm_info and inspect requests are not aborted after an error""" + with kernel() as kc: + msg_id1 = kc.execute(code="assert False") + msg_id2 = kc.kernel_info() + msg_id3 = kc.comm_info() + msg_id4 = kc.inspect(code="pri") + reply1 = kc.get_shell_msg(timeout=TIMEOUT) # execute + reply2 = kc.get_shell_msg(timeout=TIMEOUT) # kernel_info + reply3 = kc.get_shell_msg(timeout=TIMEOUT) # comm_info + reply4 = kc.get_shell_msg(timeout=TIMEOUT) # inspect + assert reply1['content']['status'] == 'error' + assert reply2['content']['status'] == 'ok' + assert reply3['content']['status'] == 'ok' + assert reply4['content']['status'] == 'ok' \ No newline at end of file From e213664980d7657640247f117a3e9e9e62d800a3 Mon Sep 17 00:00:00 2001 From: Glen Takahashi Date: Mon, 14 Dec 2020 15:47:21 -0800 Subject: [PATCH 4/6] Add newline --- ipykernel/tests/test_kernel.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ipykernel/tests/test_kernel.py b/ipykernel/tests/test_kernel.py index 9602dd737..0481b7cf6 100644 --- a/ipykernel/tests/test_kernel.py +++ b/ipykernel/tests/test_kernel.py @@ -416,4 +416,4 @@ def test_dont_abort_non_execute_requests(): assert reply1['content']['status'] == 'error' assert reply2['content']['status'] == 'ok' assert reply3['content']['status'] == 'ok' - assert reply4['content']['status'] == 'ok' \ No newline at end of file + assert reply4['content']['status'] == 'ok' From 3c652dec940fdeac47e021894fdf649cd6fbb7d2 Mon Sep 17 00:00:00 2001 From: Glen Takahashi Date: Mon, 14 Dec 2020 16:06:36 -0800 Subject: [PATCH 5/6] Add aborted message_spec --- ipykernel/tests/test_message_spec.py | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/ipykernel/tests/test_message_spec.py b/ipykernel/tests/test_message_spec.py index 51a699fe8..5ca293be1 100644 --- a/ipykernel/tests/test_message_spec.py +++ b/ipykernel/tests/test_message_spec.py @@ -114,6 +114,8 @@ def check(self, d): ExecuteReplyOkay().check(d) elif d['status'] == 'error': ExecuteReplyError().check(d) + elif d['status'] == 'aborted': + ExecuteReplyAborted().check(d) class ExecuteReplyOkay(Reply): @@ -122,11 +124,16 @@ class ExecuteReplyOkay(Reply): class ExecuteReplyError(Reply): + status = Enum(('error',)) ename = Unicode() evalue = Unicode() traceback = List(Unicode()) +class ExecuteReplyAborted(Reply): + status = Enum(('aborted',)) + + class InspectReply(Reply, MimeBundle): found = Bool() From 983d5708154061126aea2fe5ca1b32051fe723f1 Mon Sep 17 00:00:00 2001 From: Glen Takahashi Date: Mon, 14 Dec 2020 16:19:14 -0800 Subject: [PATCH 6/6] Move test --- ipykernel/tests/test_kernel.py | 26 -------------------------- ipykernel/tests/test_message_spec.py | 24 ++++++++++++++++++++++++ 2 files changed, 24 insertions(+), 26 deletions(-) diff --git a/ipykernel/tests/test_kernel.py b/ipykernel/tests/test_kernel.py index 0481b7cf6..8cfcd3b2a 100644 --- a/ipykernel/tests/test_kernel.py +++ b/ipykernel/tests/test_kernel.py @@ -391,29 +391,3 @@ def test_interrupt_during_pdb_set_trace(): validate_message(reply, 'execute_reply', msg_id) reply = kc.get_shell_msg(timeout=TIMEOUT) validate_message(reply, 'execute_reply', msg_id2) - -def test_abort_execute_requests(): - """test that execute_request's are aborted after an error""" - with kernel() as kc: - msg_id1 = kc.execute(code="assert False") - msg_id2 = kc.execute(code="assert True") - reply1 = kc.get_shell_msg(timeout=TIMEOUT) - reply2 = kc.get_shell_msg(timeout=TIMEOUT) - assert reply1['content']['status'] == 'error' - assert reply2['content']['status'] == 'aborted' - -def test_dont_abort_non_execute_requests(): - """test that kernel_info, comm_info and inspect requests are not aborted after an error""" - with kernel() as kc: - msg_id1 = kc.execute(code="assert False") - msg_id2 = kc.kernel_info() - msg_id3 = kc.comm_info() - msg_id4 = kc.inspect(code="pri") - reply1 = kc.get_shell_msg(timeout=TIMEOUT) # execute - reply2 = kc.get_shell_msg(timeout=TIMEOUT) # kernel_info - reply3 = kc.get_shell_msg(timeout=TIMEOUT) # comm_info - reply4 = kc.get_shell_msg(timeout=TIMEOUT) # inspect - assert reply1['content']['status'] == 'error' - assert reply2['content']['status'] == 'ok' - assert reply3['content']['status'] == 'ok' - assert reply4['content']['status'] == 'ok' diff --git a/ipykernel/tests/test_message_spec.py b/ipykernel/tests/test_message_spec.py index 5ca293be1..c6404a626 100644 --- a/ipykernel/tests/test_message_spec.py +++ b/ipykernel/tests/test_message_spec.py @@ -355,6 +355,30 @@ def test_execute_stop_on_error(): assert reply['content']['status'] == 'ok' +def test_non_execute_stop_on_error(): + """test that non-execute_request's are not aborted after an error""" + flush_channels() + + fail = '\n'.join([ + # sleep to ensure subsequent message is waiting in the queue to be aborted + 'import time', + 'time.sleep(0.5)', + 'raise ValueError', + ]) + KC.execute(code=fail) + KC.kernel_info() + KC.comm_info() + KC.inspect(code="print") + reply = KC.get_shell_msg(timeout=TIMEOUT) # execute + assert reply['content']['status'] == 'error' + reply = KC.get_shell_msg(timeout=TIMEOUT) # kernel_info + assert reply['content']['status'] == 'ok' + reply = KC.get_shell_msg(timeout=TIMEOUT) # comm_info + assert reply['content']['status'] == 'ok' + reply = KC.get_shell_msg(timeout=TIMEOUT) # inspect + assert reply['content']['status'] == 'ok' + + def test_user_expressions(): flush_channels()