From c5e728feda979f7d4e92413aa5a5d2aaf0cb0875 Mon Sep 17 00:00:00 2001 From: Glyph Date: Sun, 14 Jan 2024 11:30:36 -0800 Subject: [PATCH 1/6] let's try invalidating reactors and see if that gives us a hint --- src/twisted/internet/test/reactormixins.py | 13 +++++++++++++ src/twisted/newsfragments/11771.misc | 0 2 files changed, 13 insertions(+) create mode 100644 src/twisted/newsfragments/11771.misc diff --git a/src/twisted/internet/test/reactormixins.py b/src/twisted/internet/test/reactormixins.py index 506f6e73ae0..f9c827109b1 100644 --- a/src/twisted/internet/test/reactormixins.py +++ b/src/twisted/internet/test/reactormixins.py @@ -106,6 +106,13 @@ def stopIfError(event): case.addCleanup(publisher.removeObserver, stopIfError) +class InvalidatedReactor: + """ + This object has no methods and no state, and will hopefully explode if any + code attempts to interact with a reactor from a previous test. + """ + + class ReactorBuilder: """ L{SynchronousTestCase} mixin which provides a reactor-creation API. This @@ -254,6 +261,12 @@ def _unbuildReactor(self, reactor): globalReactor.__dict__ = reactor._originalReactorDict globalReactor.__class__ = reactor._originalReactorClass + # Some reactors (notably, CFReactor) manipulate global state if you + # interact with them; doing so across tests is inherently a potential + # state corruption. Let's make that more visible. + reactor.__class__ = InvalidatedReactor + reactor.__dict__ = {} + def buildReactor(self): """ Create and return a reactor using C{self.reactorFactory}. diff --git a/src/twisted/newsfragments/11771.misc b/src/twisted/newsfragments/11771.misc new file mode 100644 index 00000000000..e69de29bb2d From 31a705b438a0fed0bc852d8f29cbe9aef9706a74 Mon Sep 17 00:00:00 2001 From: Glyph Date: Sun, 14 Jan 2024 11:37:54 -0800 Subject: [PATCH 2/6] test run 2 From 896cca89138e723913f6a74829ae3e1a581683ad Mon Sep 17 00:00:00 2001 From: Glyph Date: Sun, 14 Jan 2024 11:39:00 -0800 Subject: [PATCH 3/6] test run 3 From cce48869e6f9f173f16fb839cf6b0a1e09aeb787 Mon Sep 17 00:00:00 2001 From: Glyph Date: Sun, 14 Jan 2024 11:46:45 -0800 Subject: [PATCH 4/6] hypothesis: some native CFRunLoop state gets GC'd between runs the first 2 builds in this PR resulted in 5/6 macOS builds succeeding which is seems to be slightly beating the usual odds of 50/50, and nothing has changed except for the removal of a reference to potential active CFRunLoopSource objects in the reactor under test. if we *also* garbage collect in immediately after breaking any references to CFRunLoop state, does that improve the odds further? --- src/twisted/internet/test/reactormixins.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/twisted/internet/test/reactormixins.py b/src/twisted/internet/test/reactormixins.py index f9c827109b1..3464ec4e5cf 100644 --- a/src/twisted/internet/test/reactormixins.py +++ b/src/twisted/internet/test/reactormixins.py @@ -15,6 +15,7 @@ __all__ = ["TestTimeoutError", "ReactorBuilder", "needsRunningReactor"] +import gc import os import signal import time @@ -266,6 +267,7 @@ def _unbuildReactor(self, reactor): # state corruption. Let's make that more visible. reactor.__class__ = InvalidatedReactor reactor.__dict__ = {} + gc.collect() def buildReactor(self): """ From 011dc2b997a3cc8e11e3f0c0aabe4329dffb187b Mon Sep 17 00:00:00 2001 From: Glyph Date: Sun, 14 Jan 2024 12:10:22 -0800 Subject: [PATCH 5/6] tell me your secrets --- src/twisted/internet/cfreactor.py | 31 ++++++++++++++++++------------- 1 file changed, 18 insertions(+), 13 deletions(-) diff --git a/src/twisted/internet/cfreactor.py b/src/twisted/internet/cfreactor.py index f686092e69d..ce72c9ddefe 100644 --- a/src/twisted/internet/cfreactor.py +++ b/src/twisted/internet/cfreactor.py @@ -197,11 +197,11 @@ def _drdw(): # reading/writing state flags to determine whether we # should actually attempt a doRead/doWrite first. -glyph if isRead: - if rw[_READ]: - why = readWriteDescriptor.doRead() + assert rw[_READ], "read callback when twisted not reading" + why = readWriteDescriptor.doRead() else: - if rw[_WRITE]: - why = readWriteDescriptor.doWrite() + assert rw[_WRITE], "write callback when twisted not writing" + why = readWriteDescriptor.doWrite() except BaseException: why = sys.exc_info()[1] log.err() @@ -225,10 +225,12 @@ def _watchFD(self, fd, descr, flag): @param flag: the flag to register for callbacks on, either C{kCFSocketReadCallBack} or C{kCFSocketWriteCallBack} """ + assert isinstance(fd, int), f"only ints allowed, not {fd!r}" if fd == -1: raise RuntimeError("Invalid file descriptor.") if fd in self._fdmap: src, cfs, gotdescr, rw = self._fdmap[fd] + assert gotdescr is descr, f"mismatch between {descr} and {gotdescr}" # do I need to verify that it's the same descr? else: ctx = [] @@ -295,18 +297,21 @@ def _unwatchFD(self, fd, descr, flag): @param flag: C{kCFSocketWriteCallBack} C{kCFSocketReadCallBack} """ - if id(descr) not in self._idmap: + descrid = id(descr) + if descrid not in self._idmap: return - if fd == -1: - # need to deal with it in this case, I think. - realfd = self._idmap[id(descr)] - else: - realfd = fd - src, cfs, descr, rw = self._fdmap[realfd] + realfd = self._idmap[descrid] + src, cfs, gotdescr, rw = self._fdmap[realfd] + assert ( + gotdescr is descr + ), f"unwatching file descriptor mismatch {descr!r} {gotdescr!r}" CFSocketDisableCallBacks(cfs, flag) rw[self._flag2idx(flag)] = False - if not rw[_READ] and not rw[_WRITE]: - del self._idmap[id(descr)] + assert ( + fd == realfd + ), f"file descriptor invalidated while running {fd!r} {realfd!r}" + if fd != realfd or (not rw[_READ] and not rw[_WRITE]): + del self._idmap[descrid] del self._fdmap[realfd] CFRunLoopRemoveSource(self._cfrunloop, src, kCFRunLoopCommonModes) CFSocketInvalidate(cfs) From 010f95e9ece28b6b1b1a1c0ed4a343d7aaf490b8 Mon Sep 17 00:00:00 2001 From: Glyph Date: Mon, 15 Jan 2024 12:59:22 -0800 Subject: [PATCH 6/6] remove assert --- src/twisted/internet/cfreactor.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/twisted/internet/cfreactor.py b/src/twisted/internet/cfreactor.py index ce72c9ddefe..d80d8f051a4 100644 --- a/src/twisted/internet/cfreactor.py +++ b/src/twisted/internet/cfreactor.py @@ -307,9 +307,6 @@ def _unwatchFD(self, fd, descr, flag): ), f"unwatching file descriptor mismatch {descr!r} {gotdescr!r}" CFSocketDisableCallBacks(cfs, flag) rw[self._flag2idx(flag)] = False - assert ( - fd == realfd - ), f"file descriptor invalidated while running {fd!r} {realfd!r}" if fd != realfd or (not rw[_READ] and not rw[_WRITE]): del self._idmap[descrid] del self._fdmap[realfd]