Repository navigation
Fix SuppressStdoutStderr permanently silencing the process when contexts overlap - #3290
Open
choksi2212 wants to merge 2 commits into
Open
choksi2212 wants to merge 2 commits into
choksi2212 wants to merge 2 commits into
Conversation
The context manager duplicated and restored the process-level stdout and stderr descriptors per instance, with no synchronization. A second context created while a first one was active saved the devnull descriptors as if they were the real ones, and whoever exited last dup2-ed devnull back onto fd 1 and 2, permanently silencing the process (unit8co#3276). Constructing it in interpreters without real standard streams (pythonw, GUI hosts) also raised OSError from __init__ and leaked the descriptors opened before the failure. The redirection is now shared: a module-level lock plus a depth counter, the outermost context captures and restores the real descriptors, contexts entered while a redirection is active just join it, and all fd work moved into __enter__ with a graceful no-op fallback when the standard streams cannot be saved. Signed-off-by: choksi2212 <choksi2212@users.noreply.github.com>
Signed-off-by: choksi2212 <choksi2212@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #3290 +/- ##
=======================================
Coverage 97.13% 97.13%
=======================================
Files 175 175
Lines 19408 19447 +39
=======================================
+ Hits 18851 18890 +39
Misses 557 557 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
SuppressStdoutStderrused to duplicate fd 1 and 2 in__init__and restore them in__exit__, per instance, with no synchronization. If a second context was created while a first one was active (two threads fitting Prophet models side by side, or just overlappingwithblocks), the second one saved the devnull descriptors as if they were the real ones, and whoever exited last pointed fd 1 and 2 back at devnull for the rest of the process lifetime — the scenario from #3276, where after two overlapping fits everything printed afterwards was gone. Creating the context in interpreters without usable standard streams (pythonw on Windows, GUI-embedded hosts) also raisedOSError: Bad file descriptorstraight from__init__, leaking the descriptors opened just before the failure.The redirection is now shared state: a module-level lock plus a depth counter, where the outermost context captures and restores the real descriptors, contexts entered while a redirection is already active just join it, and all descriptor work happens in
__enter__with a graceful no-op fallback when the standard streams cannot be saved. I left Prophet's suppression path as-is — it becomes safe through the context manager itself, and switching it over to cmdstanpy logger levels felt like a separate design call.How I checked it: on master, the new threaded test fails exactly like the issue describes (
assert b'' == b'survivor'— the write after both contexts lands nowhere) and the fallback test dies withOSError: [Errno 9] Bad file descriptorout of__init__. With the fix both pass, along with the round-trip and nested tests and the rest oftest_logging.py(8 passed). The overlap choreography is event-driven rather than sleep-based, so the interleaving is deterministic.ruff checkandruff format(0.7.2, same as pre-commit) are clean.Closes #3276.