Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for operation on closed file in faulthandler teardown #11584

Merged
merged 6 commits into from
Nov 22, 2023

Conversation

bnomis
Copy link
Contributor

@bnomis bnomis commented Nov 3, 2023

Closes #11572

remember stderr fileno in config.stash to be restored on faulthandler teardown

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a good change 👍 .

I think we should simplify a bit by combining fault_handler_original_stderr_fd_key and fault_handler_originally_enabled_key; specifically, scrape fault_handler_originally_enabled_key and use fault_handler_original_stderr_fd_key to see if faulthandler was enabled before or not.

Also agree with @RonnyPfannschmidt that a comment explaining that, in case the faulthandler is already enabled, the faulthandler api doesn't provide a way to get the FD that was previously passed to enable(), so we assume it was the default (stderr).

@bnomis
Copy link
Contributor Author

bnomis commented Nov 13, 2023

Can this PR be merged? Or is there more specific changes needed? Thanks.

@bluetech
Copy link
Member

@bnomis There are some unresolved comments above - add a comment and combine the keys (see my comment above), address the coverage issue mentioned by @nicoddemus.

@bnomis
Copy link
Contributor Author

bnomis commented Nov 21, 2023

@bluetech thank you for your comments. I have updated the PR.

Copy link
Member

@bluetech bluetech left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @bnomis!

@bluetech bluetech merged commit a42530a into pytest-dev:main Nov 22, 2023
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ValueError: I/O operation on closed file
4 participants