Fix logcontext leak from test_sync #4213
Conversation
Avoid timing out the request, which for long and complicated reasons causes logcontext leaks,
| # it completes, we can end up leaking log-contexts (see log_contexts.rst | ||
| # for more details). | ||
| # | ||
| # In short, a timeout here makes things explode down the line. |
There was a problem hiding this comment.
Lack of a timeout here causes an infinite loop on jammed web requests in tests...
There was a problem hiding this comment.
doesn't the test have an overall timeout?
There was a problem hiding this comment.
I guess I don't mind having an explicit timeout here, but what is a problem is making it an expected part of a test.
There was a problem hiding this comment.
The timeout is done by the global reactor, this here does not yield to the global reactor, and therefore can run infinitely and not trip the global reactor timeout.
There was a problem hiding this comment.
well, that's sad, but given that if we're in that situation the test should have failed, is it really a problem?
There was a problem hiding this comment.
I don't think I like this approach. Should we not instead, in tests.unittest.HomeserverTestCase, reset the log context to None either during setUp, tearDown, or both? Hanging Deferreds, due to how many moving parts there are in the HomeServer, are likely to happen often.
|
the problem is that the logcontext is already clear during setup and teardown. It is then later restored - in the middle of some completely random test - by the orphaned deferred chain being garbage-collected. I'm with you, it's not a nice approach. But I can't really think of any better ones right now. Options might be:
|
|
@richvdh Can we force a GC between tests to cause the teardown to happen immediately? |
|
possibly. It feels horrible though. |
|
ok fine. Have done #4227 instead. |
Avoid timing out the request, which for long and complicated reasons causes logcontext leaks.