Fixes #415 - Close stdin when tearing down subprocesses in system-tests to prevent fd leaks #505
Conversation
…sts to prevent fd leaks
Make sure a router exits if an initial HTTP listener fails, doesn't hang ... FAIL /usr/lib64/python3.10/unittest/case.py:613: ResourceWarning: unclosed file <_io.BufferedWriter name=6> outcome.errors.clear() ResourceWarning: Enable tracemalloc to get the object allocation traceback
Codecov Report
@@ Coverage Diff @@
## main #505 +/- ##
==========================================
- Coverage 25.17% 25.17% -0.01%
==========================================
Files 115 115
Lines 28772 28772
Branches 4669 4669
==========================================
- Hits 7243 7242 -1
Misses 20562 20562
- Partials 967 968 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
| @@ -372,6 +372,10 @@ def error(msg): | |||
| # at this point the process has terminated, either of its own accord or | |||
| # due to the above terminate call. | |||
|
|
|||
| # prevent leak of stdin fd | |||
There was a problem hiding this comment.
Is closing the stdin the only fix needed in order not to see the backtrace seen in #415 ?
There was a problem hiding this comment.
Yes, close stdin and ensure that the teardown method is always called.
There was a problem hiding this comment.
Ok, so you added the __enter__ and __exit__ functions to class Qdrouterd so if anyone uses the with keyword, we can ensure that the teardown() function will be called. But adding these two functions and the changes to the other tests in this commit are not part of the fix. The actual fix is to close the stdin on teardown(), right ?
There was a problem hiding this comment.
It is part of the fix. I need to ensure that stdin gets closed. If some tests never called teardown() and I am closing stdin in teardown, then I have to also call teardown there. And I can best do it with the with thing.
Am I missing something obvious here?
There was a problem hiding this comment.
I think I get it now. The change in system_tests_management.py from assertTrue to assertEqual is unrelated, yes. Let me put it to separate PR.
There was a problem hiding this comment.
Split into #508, thanks!
jiridanek
force-pushed
the
jd_2022_05_26_close_stdin_subprocess
branch
from
1d4bbb9
to
102945b
Compare
No description provided.