twisted / twisted

Event-driven networking engine written in Python.
https://www.twisted.org
Other
5.44k stars 1.14k forks source link

#11771 attempt a fix for cfreactor flakiness #12150

Open glyph opened 2 weeks ago

glyph commented 2 weeks ago

Scope and purpose

Fixes #11771

glyph commented 2 weeks ago

The fact that twisted.internet.test.test_fdset.ReactorFDSetTestsBuilder_CFReactorTests.test_connectionLostOnShutdown failed is interesting. That test does nothing but start up, add a couple of file descriptors which never do anything, then shut itself down. The fact that it's hanging suggests that there's an error in the logic that sets up _scheduleSimulate to always run.

glyph commented 2 weeks ago

OK, same test still flaky. The fact that the reactor always says that it couldn't stop a reactor that is already running suggests that somehow the application's call to reactor.stop() is happening, but it's not happening until the reactor wakes up for the framework's timeout stop().

I guess I will try to do some print statement debugging in CI to validate this theory.

glyph commented 2 weeks ago

dangit fail

glyph commented 2 weeks ago

The failure on try 2 is this

===============================================================================
[FAIL]
Traceback (most recent call last):
  File "/Users/runner/work/twisted/twisted/.tox/macos-withcov-alldeps/lib/python3.12/site-packages/twisted/internet/test/test_udp.py", line 103, in test_connectionLostLogMessage
    self.assertEqual((expectedMessage,), loggedMessages[0]["message"])
  File "/Users/runner/work/twisted/twisted/.tox/macos-withcov-alldeps/lib/python3.12/site-packages/twisted/trial/_synctest.py", line 444, in assertEqual
    super().assertEqual(first, second, msg)
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/unittest/case.py", line 885, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/unittest/case.py", line 1102, in assertTupleEqual
    self.assertSequenceEqual(tuple1, tuple2, msg, seq_type=tuple)
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/unittest/case.py", line 1073, in assertSequenceEqual
    self.fail(msg)
twisted.trial.unittest.FailTest: Tuples differ: ('(UDP Port 63035 Closed)',) != ()

First tuple contains 1 additional elements.
First extra element 0:
'(UDP Port 63035 Closed)'

- ('(UDP Port 63035 Closed)',)
+ ()

twisted.internet.test.test_udp.UDPFDServerTestsBuilder_CFReactorTests.test_connectionLostLogMessage

which… coincidentally is on the CF reactor, but seems to be unrelated to the core issue here

glyph commented 2 weeks ago

The failure on try 3 is similar:

Traceback (most recent call last):
  File "/Users/runner/work/twisted/twisted/.tox/macos-withcov-alldeps/lib/python3.12/site-packages/twisted/internet/test/test_udp.py", line 103, in test_connectionLostLogMessage
    self.assertEqual((expectedMessage,), loggedMessages[0]["message"])
  File "/Users/runner/work/twisted/twisted/.tox/macos-withcov-alldeps/lib/python3.12/site-packages/twisted/trial/_synctest.py", line 444, in assertEqual
    super().assertEqual(first, second, msg)
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/unittest/case.py", line 885, in assertEqual
    assertion_func(first, second, msg=msg)
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/unittest/case.py", line 1102, in assertTupleEqual
    self.assertSequenceEqual(tuple1, tuple2, msg, seq_type=tuple)
  File "/Library/Frameworks/Python.framework/Versions/3.12/lib/python3.12/unittest/case.py", line 1073, in assertSequenceEqual
    self.fail(msg)
twisted.trial.unittest.FailTest: Tuples differ: ('(UDP Port 57724 Closed)',) != ()

First tuple contains 1 additional elements.
First extra element 0:
'(UDP Port 57724 Closed)'

- ('(UDP Port 57724 Closed)',)
+ ()

twisted.internet.test.test_udp.UDPFDServerTestsBuilder_CFReactorTests.test_connectionLostLogMessage

… still the CF reactor, again, which I don't love. Will investigate but I think it's just a different flake

glyph commented 2 weeks ago

Converting this to a draft PR because I am pretty sure #12160 fixed the actual issue here, but this branch contains some useful code (particularly the disttrial debug-print stuff, which should be spun out into a PR addressing #12163 )