Deprecation/removal of @json_errors prevents return of NoSuchKernel exception

[kevin-bates]

The kernel_gateway (and, by extension the new enterprise_gateway incubator) tests now encounter a test failure which is specifically looking for the NoSuchKernel exception in the returned json body.

The test issues a start kernel request sans the kernel name, after setting the default kernel name to a bogus value (fake-kernel) and expects an exception - knowing that fake-kernel doesn't exist, which then confirms that the default kernel is being used. However, after 5.2.0 has been available, all build tests get this single failure. I then used some targeted searches and pulled various commits to see where the breakage occurs and found that the commits in PR #2853 lead to the issue.

Unfortunately, I'm not certain how callers are supposed to address this issue since I'm not familiar enough with tornado handlers.

Here's the command to reproduce the issue from JKG: nosetests --nocapture --cover-package=kernel_gateway kernel_gateway.tests.test_jupyter_websocket.TestCustomDefaultKernel

and output...

F
======================================================================
FAIL: The default kernel name should be used on empty requests.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/opt/anaconda/envs/enterprise-gateway-dev/lib/python3.6/site-packages/tornado/testing.py", line 136, in __call__
    result = self.orig_method(*args, **kwargs)
  File "/opt/anaconda/envs/enterprise-gateway-dev/lib/python3.6/site-packages/tornado/testing.py", line 529, in post_coroutine
    timeout=timeout)
  File "/opt/anaconda/envs/enterprise-gateway-dev/lib/python3.6/site-packages/tornado/ioloop.py", line 458, in run_sync
    return future_cell[0].result()
  File "/opt/anaconda/envs/enterprise-gateway-dev/lib/python3.6/site-packages/tornado/concurrent.py", line 238, in result
    raise_exc_info(self._exc_info)
  File "<string>", line 4, in raise_exc_info
  File "/opt/anaconda/envs/enterprise-gateway-dev/lib/python3.6/site-packages/tornado/gen.py", line 1069, in run
    yielded = self.gen.send(value)
  File "/opt/anaconda/envs/enterprise-gateway-dev/lib/python3.6/types.py", line 182, in send
    return self.__wrapped.send(val)
  File "/Users/kbates/repos/oss/jupyter/kernel_gateway/kernel_gateway/tests/test_jupyter_websocket.py", line 574, in test_default_kernel_name
    self.assertTrue('raise NoSuchKernel' in str(response.body))
AssertionError: False is not true

----------------------------------------------------------------------
Ran 1 test in 0.213s

FAILED (failures=1)

I added a print statement into the corresponding enterprise_gateway test to see the actual response body. response.body: b'{"reason": "Internal Server Error", "message": ""}' when a fairly rich stack trace has been previously included.

[takluyver]

It looks like the notebook code in write_error is doing this:

if isinstance(e, HTTPError):
     reply['message'] = e.log_message or message
else:
     reply['message'] = 'Unhandled error'
     reply['traceback'] = ''.join(traceback.format_exception(*exc_info))

Whereas the equivalent code in kernel_gateway (in JSONErrorsMixin) looks like this:

try:
     message = exception.log_message % exception.args
except Exception:
     pass

I suspect that's the source of the difference.

[kevin-bates]

Thanks @takluyver (and congratulations on your recent recognition!)

So if I understand your response, it seems to imply that the JSONErrorsMixin code had been previously bypassed due to the presence of @json_errors and, now that the deprecation has taken place, JSONErrorsMixin is not performing a compatible response that now causes the unit test to break. Correct?

As a result, are you suggesting the kernel gateway code get updated?

Thanks.

[takluyver]

I haven't really followed kernel_gateway closely, and I don't know exactly how the deprecation of json_errors has affected it. But the write_errors method is meant to be used instead of the decorator, and it looks like that method in notebook is putting more information into the JSON than the method in kernel_gateway.

[kevin-bates]

Thanks @takluyver. I'm pretty sure I see what's going on here - adding @minrk and @parente for comment.

Once the json_errors decorator was removed, this caused the JSONErrorsMixin.write_error to be called in JKG instead of APIHandler.write_error due to inheritance rules. However, JSONErrorsMixin doesn't deal with tracebacks and essentially ignores any non-HTTP kinds of exceptions - like NoSuchKernel - leading to test failures.

Since APIHandler is in the handler class hierarchy of JKG classes, it stands to reason that we could now get rid of JSONErrorsMixin.write_error now that APIHandler.write_error exists and handles both HTTP and non-HTTP exceptions. However, there's one issue that I believe was an oversight in the #2853 and that's that APIHandler.write_error does not set the reason for the reply, whereas @json_errors did that before.

I stumbled upon this when looking at another test failure where the reason field for a 403 exception was not the expected Forbidden but instead None. In looking at JSONErrorsMixin it makes the following call: reason = responses.get(status_code, 'Unknown HTTP Error'). But, APIHandler.write_error essentially does the equivalent when initializing the message field, only to overwrite that field with info from the exc_info structure (if it exists). As a result, I need to ask if APIHandler.write_error should be using reason where message is used in these lines: https://github.com/jupyter/notebook/blob/master/notebook/base/handlers.py#L461-L464

If I make that change, then all JKG tests pass when JSONErrorsMixin is removed from the class hierarchy.

The other approach is to apply the content of APIHandler.write_error - along with the "reason update" I raised - directly into JSONErrorsMixin.write_error and just live with retaining JSONErrorsMixin in JKG. I think we'll need to do this anyway in order to bridge this gap from a release perspective in the near term anyway.

[kevin-bates]

Sorry for all the ramble (that's what I do :-) ). Now that this has been narrowed down and I'm convinced the notebook portion is just a matter of establishing the reason in the error, I'm going to close this issue and open a new one that is more succinct and to the point. This will be followed up with a pull request.