Swallow potential exceptions from showtraceback()#13934
Merged
Carreau merged 3 commits intoipython:mainfrom Feb 10, 2023
Merged
Swallow potential exceptions from showtraceback()#13934Carreau merged 3 commits intoipython:mainfrom
Carreau merged 3 commits intoipython:mainfrom
Conversation
The nbgrader project is aware of a form of cheating where students disrupt `InteractiveShell.showtraceback` in hopes of hiding exceptions to avoid losing points. They have implemented a solution to prevent this cheating from working on the client side, and have some tests to demonstrate this technique: https://github.com/jupyter/nbgrader/blob/main/nbgrader/tests/apps/files/submitted-cheat-attempt.ipynb https://github.com/jupyter/nbgrader/blob/main/nbgrader/tests/apps/files/submitted-cheat-attempt-alternative.ipynb In essence, these attacks import the interactive shell and erase the traceback handler so that their failing tests won't report failures. import IPython.core.interactiveshell IPython.core.interactiveshell.InteractiveShell.showtraceback = None The problem is that this causes an exception inside the kernel, leading to a stalled execution. The kernel has stopped working, but the client continues to wait for messages. So far, nbgrader's solution to this is to require a timeout value so the client can eventually decide it is done. This prevents allowing a value of `None` for `Execute.timeout` because this would cause a test case to infinitely hang. This commit addresses the problem by making `InteractiveShell._run_cell` a little more protective around it's call to `showtraceback()`. There is already a try/except block around running the cell. This commit adds a finally clause so that the method will _always_ return an `ExecutionResult`, even if a new exception is thrown within the except clause. For the record, the exception thrown is: TypeError: 'NoneType' object is not callable Accepting this change will allow nbgrader to update `nbgrader.preprocessors.Execute` to support a type of `Integer(allow_none=True)` as the parent `NotebookClient` intended. Discussion about this is ongoing in jupyter/nbgrader#1690.
Contributor
Author
|
Of course! The tests all pass on my machine, so I have no idea why they're failing in CI. Converting to draft until I figure this out. |
Member
|
That looks good to me, as test are now passing I'm marking as ready to merge, I'll leave the be for a few hours/a day and will merge, unless someone else stop by and merge in the meantime. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The nbgrader project is aware of a form of cheating where students disrupt
InteractiveShell.showtracebackin hopes of hiding exceptions to avoid losing points. They have implemented a solution to prevent this cheating from working on the client side, and have some tests to demonstrate this technique:https://github.com/jupyter/nbgrader/blob/main/nbgrader/tests/apps/files/submitted-cheat-attempt.ipynb https://github.com/jupyter/nbgrader/blob/main/nbgrader/tests/apps/files/submitted-cheat-attempt-alternative.ipynb
In essence, these attacks import the interactive shell and erase the traceback handler so that their failing tests won't report failures.
The problem is that this causes an exception inside the kernel, leading to a stalled execution. The kernel has stopped working, but the client continues to wait for messages. So far, nbgrader's solution to this is to require a timeout value so the client can eventually decide it is done. This prevents allowing a value of
NoneforExecute.timeoutbecause this would cause a test case to infinitely hang.This commit addresses the problem by making
InteractiveShell._run_cella little more protective around it's call toshowtraceback(). There is already a try/except block around running the cell. This commit adds a finally clause so that the method will always return anExecutionResult, even if a new exception is thrown within the except clause. For the record, the exception thrown is:Accepting this change will allow nbgrader to update
nbgrader.preprocessors.Executeto support a type ofInteger(allow_none=True)as the parentNotebookClientintended.Discussion about this is ongoing in jupyter/nbgrader#1690.