Repository: twisted/twisted

Issue:
twisted.conch.manhole fails to display tracebacks correctly if sys.excepthook has been modified
manhole inherits from `code.InteractiveInterpreter` to handle some of its REPL-y behavior.  `code.InteractiveInterpreter` varies some of its behavior on whether `sys.excepthook` has been fiddled with.  manhole assumes it will _always_ get the behavior you get if `sys.excepthook` hasn't been tampered with so it breaks (and so does one of its unit tests) if it has been tampered with.

This is an obscure hook that's mostly left alone (which is why so many years passed before it was a problem, I suppose).  An example of a library that does stuff with `sys.excepthook` is `exceptiongroup`.  It turns out Hypothesis depends on `exceptiongroup` so if we try to use Hypothesis in Twisted then `sys.excepthook` gets tampered with an manhole tests start failing.

Here's an exceptiongroup bug report - https://github.com/agronholm/exceptiongroup/issues/26 - but whether this is fixable in exceptiongroup or not is unclear (seems like perhaps not).  Probably the stdlib should stop having this wonky conditional but ...


Discussion:

Patch:
diff --git a/src/twisted/conch/manhole.py b/src/twisted/conch/manhole.py
index f26e5de974..5bf2f817a4 100644
--- a/src/twisted/conch/manhole.py
+++ b/src/twisted/conch/manhole.py
@@ -17,11 +17,15 @@ import code
 import sys
 import tokenize
 from io import BytesIO
+from traceback import format_exception
+from types import TracebackType
+from typing import Type
 
 from twisted.conch import recvline
 from twisted.internet import defer
 from twisted.python.compat import _get_async_param
 from twisted.python.htmlizer import TokenPrinter
+from twisted.python.monkey import MonkeyPatcher
 
 
 class FileWrapper:
@@ -71,6 +75,11 @@ class ManholeInterpreter(code.InteractiveInterpreter):
         self.filename = filename
         self.resetBuffer()
 
+        self.monkeyPatcher = MonkeyPatcher()
+        self.monkeyPatcher.addPatch(sys, "displayhook", self.displayhook)
+        self.monkeyPatcher.addPatch(sys, "excepthook", self.excepthook)
+        self.monkeyPatcher.addPatch(sys, "stdout", FileWrapper(self.handler))
+
     def resetBuffer(self):
         """
         Reset the input buffer.
@@ -104,15 +113,20 @@ class ManholeInterpreter(code.InteractiveInterpreter):
         return more
 
     def runcode(self, *a, **kw):
-        orighook, sys.displayhook = sys.displayhook, self.displayhook
-        try:
-            origout, sys.stdout = sys.stdout, FileWrapper(self.handler)
-            try:
-                code.InteractiveInterpreter.runcode(self, *a, **kw)
-            finally:
-                sys.stdout = origout
-        finally:
-            sys.displayhook = orighook
+        with self.monkeyPatcher:
+            code.InteractiveInterpreter.runcode(self, *a, **kw)
+
+    def excepthook(
+        self,
+        excType: Type[BaseException],
+        excValue: BaseException,
+        excTraceback: TracebackType,
+    ) -> None:
+        """
+        Format exception tracebacks and write them to the output handler.
+        """
+        lines = format_exception(excType, excValue, excTraceback.tb_next)
+        self.write("".join(lines))
 
     def displayhook(self, obj):
         self.locals["_"] = obj
diff --git a/src/twisted/conch/newsfragments/11638.bugfix b/src/twisted/conch/newsfragments/11638.bugfix
new file mode 100644
index 0000000000..d97998919b
--- /dev/null
+++ b/src/twisted/conch/newsfragments/11638.bugfix
@@ -0,0 +1,1 @@
+twisted.conch.manhole.ManholeInterpreter now captures tracebacks even if sys.excepthook has been modified.
diff --git a/src/twisted/python/monkey.py b/src/twisted/python/monkey.py
index dbf2dca7d8..08ccef2ac1 100644
--- a/src/twisted/python/monkey.py
+++ b/src/twisted/python/monkey.py
@@ -48,6 +48,8 @@ class MonkeyPatcher:
                 self._originals.append((obj, name, getattr(obj, name)))
             setattr(obj, name, value)
 
+    __enter__ = patch
+
     def restore(self):
         """
         Restore all original values to any patched objects.
@@ -56,6 +58,9 @@ class MonkeyPatcher:
             obj, name, value = self._originals.pop()
             setattr(obj, name, value)
 
+    def __exit__(self, excType=None, excValue=None, excTraceback=None):
+        self.restore()
+
     def runWithPatches(self, f, *args, **kw):
         """
         Apply each patch already specified. Then run the function f with the


Test patch:
diff --git a/src/twisted/conch/test/test_manhole.py b/src/twisted/conch/test/test_manhole.py
index ef07bd24bc..54159af8af 100644
--- a/src/twisted/conch/test/test_manhole.py
+++ b/src/twisted/conch/test/test_manhole.py
@@ -8,6 +8,7 @@
 Tests for L{twisted.conch.manhole}.
 """
 
+import sys
 import traceback
 from typing import Optional
 
@@ -148,9 +149,6 @@ class WriterTests(unittest.TestCase):
 class ManholeLoopbackMixin:
     serverProtocol = manhole.ColoredManhole
 
-    def wfd(self, d):
-        return defer.waitForDeferred(d)
-
     def test_SimpleExpression(self):
         """
         Evaluate simple expression.
@@ -244,10 +242,21 @@ class ManholeLoopbackMixin:
                     + defaultFunctionName.encode("utf-8"),
                     b"Exception: foo bar baz",
                     b">>> done",
-                ]
+                ],
             )
 
-        return done.addCallback(finished)
+        done.addCallback(finished)
+        return done
+
+    def test_ExceptionWithCustomExcepthook(
+        self,
+    ):
+        """
+        Raised exceptions are handled the same way even if L{sys.excepthook}
+        has been modified from its original value.
+        """
+        self.patch(sys, "excepthook", lambda *args: None)
+        return self.test_Exception()
 
     def test_ControlC(self):
         """
diff --git a/src/twisted/conch/test/test_recvline.py b/src/twisted/conch/test/test_recvline.py
index 03ac465988..3fd0157e2d 100644
--- a/src/twisted/conch/test/test_recvline.py
+++ b/src/twisted/conch/test/test_recvline.py
@@ -473,15 +473,7 @@ class _BaseMixin:
     def _assertBuffer(self, lines):
         receivedLines = self.recvlineClient.__bytes__().splitlines()
         expectedLines = lines + ([b""] * (self.HEIGHT - len(lines) - 1))
-        self.assertEqual(len(receivedLines), len(expectedLines))
-        for i in range(len(receivedLines)):
-            self.assertEqual(
-                receivedLines[i],
-                expectedLines[i],
-                b"".join(receivedLines[max(0, i - 1) : i + 1])
-                + b" != "
-                + b"".join(expectedLines[max(0, i - 1) : i + 1]),
-            )
+        self.assertEqual(receivedLines, expectedLines)
 
     def _trivialTest(self, inputLine, output):
         done = self.recvlineClient.expect(b"done")
diff --git a/src/twisted/test/test_monkey.py b/src/twisted/test/test_monkey.py
index 6bae7170cc..40bae09527 100644
--- a/src/twisted/test/test_monkey.py
+++ b/src/twisted/test/test_monkey.py
@@ -152,3 +152,22 @@ class MonkeyPatcherTests(unittest.SynchronousTestCase):
         self.assertRaises(RuntimeError, self.monkeyPatcher.runWithPatches, _)
         self.assertEqual(self.testObject.foo, self.originalObject.foo)
         self.assertEqual(self.testObject.bar, self.originalObject.bar)
+
+    def test_contextManager(self):
+        """
+        L{MonkeyPatcher} is a context manager that applies its patches on
+        entry and restore original values on exit.
+        """
+        self.monkeyPatcher.addPatch(self.testObject, "foo", "patched value")
+        with self.monkeyPatcher:
+            self.assertEqual(self.testObject.foo, "patched value")
+        self.assertEqual(self.testObject.foo, self.originalObject.foo)
+
+    def test_contextManagerPropagatesExceptions(self):
+        """
+        Exceptions propagate through the L{MonkeyPatcher} context-manager
+        exit method.
+        """
+        with self.assertRaises(RuntimeError):
+            with self.monkeyPatcher:
+                raise RuntimeError("something")