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

Jump to bottom

gh-93353: Fix importlib.resources._tempfile() finalizer #93377

Merged
merged 1 commit into from Jun 13, 2022
Merged

gh-93353: Fix importlib.resources._tempfile() finalizer #93377

merged 1 commit into from Jun 13, 2022
+8 −2

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented May 31, 2022

Fix the importlib.resources.as_file() context manager to remove the
temporary file if destroyed late during Python finalization: keep a
local reference to the os.remove() function. Patch by Victor Stinner.

@vstinner vstinner mentioned this pull request May 31, 2022
Open
@vstinner vstinner marked this pull request as draft May 31, 2022
@vstinner
Copy link
Member Author

@vstinner vstinner commented May 31, 2022

I convert the PR to a draft, it lacks an unit test. I plan to write a test.support helper function to make sure that an object is destroyed as late as possible during Python finalization. The first implementation would use os.register_at_fork().

But I also would like to change when os.register_at_fork() is cleared. IMO it's cleared too late, causing bugs like #93353 which are very hard to debug since Python cannot log errors anymore. Too many things are cleared/finalized to be able to log an error at this point. The point of the helper is to find a new hook if os.register_at_fork() is cleared earlier ;-)

Also, I'm writing in a train and I'm close to the train station, so I will finish later :-D

@vstinner
Copy link
Member Author

@vstinner vstinner commented May 31, 2022

cc @jaraco @warsaw @serhiy-storchaka

@vstinner
Copy link
Member Author

@vstinner vstinner commented May 31, 2022

Until I write an automated test, you can test the fix manually: #93353 (comment)

@vstinner vstinner mentioned this pull request Jun 13, 2022
Open
@vstinner
Copy link
Member Author

@vstinner vstinner commented Jun 13, 2022

I prepared PR #93776 to detect such bug and I prepared PR #93774 to write a test for this change.

I merged this PR to fix the issue, and once the other PRs will be merged, I will write an unit test for this fix.

@vstinner vstinner marked this pull request as ready for review Jun 13, 2022
@vstinner
pythongh-93353: Fix importlib.resources._tempfile() finalizer
e2f0c3f 
Fix the importlib.resources.as_file() context manager to remove the
temporary file if destroyed late during Python finalization: keep a
local reference to the os.remove() function. Patch by Victor Stinner.
@vstinner
Copy link
Member Author

@vstinner vstinner commented Jun 13, 2022

I rebased my PR, removed debug logs, and I marked the added parameter as keyword-only.

@carlbordum
Copy link
Contributor

@carlbordum carlbordum commented Jun 13, 2022

LGTM, great debugging :D

warsaw
warsaw approved these changes Jun 13, 2022
View changes
@vstinner vstinner merged commit 443ca73 into python:main Jun 13, 2022
13 checks passed
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Jun 13, 2022

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Jun 13, 2022

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@vstinner vstinner deleted the resources_finalizer branch Jun 13, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jun 13, 2022
@vstinner @miss-islington
pythongh-93353: Fix importlib.resources._tempfile() finalizer (python…
3c96288 
…GH-93377)

Fix the importlib.resources.as_file() context manager to remove the
temporary file if destroyed late during Python finalization: keep a
local reference to the os.remove() function. Patch by Victor Stinner.
(cherry picked from commit 443ca73)

Co-authored-by: Victor Stinner <vstinner@python.org>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jun 13, 2022

GH-93780 is a backport of this pull request to the 3.10 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jun 13, 2022
@vstinner @miss-islington
pythongh-93353: Fix importlib.resources._tempfile() finalizer (python…
f63fa00 
…GH-93377)

Fix the importlib.resources.as_file() context manager to remove the
temporary file if destroyed late during Python finalization: keep a
local reference to the os.remove() function. Patch by Victor Stinner.
(cherry picked from commit 443ca73)

Co-authored-by: Victor Stinner <vstinner@python.org>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jun 13, 2022

GH-93781 is a backport of this pull request to the 3.11 branch.

miss-islington added a commit that referenced this issue Jun 13, 2022
@miss-islington @vstinner
gh-93353: Fix importlib.resources._tempfile() finalizer (GH-93377)
dc6dd8e 
Fix the importlib.resources.as_file() context manager to remove the
temporary file if destroyed late during Python finalization: keep a
local reference to the os.remove() function. Patch by Victor Stinner.
(cherry picked from commit 443ca73)

Co-authored-by: Victor Stinner <vstinner@python.org>
miss-islington added a commit that referenced this issue Jun 13, 2022
@miss-islington @vstinner
gh-93353: Fix importlib.resources._tempfile() finalizer (GH-93377)
f9585e2 
Fix the importlib.resources.as_file() context manager to remove the
temporary file if destroyed late during Python finalization: keep a
local reference to the os.remove() function. Patch by Victor Stinner.
(cherry picked from commit 443ca73)

Co-authored-by: Victor Stinner <vstinner@python.org>
@vstinner
Copy link
Member Author

@vstinner vstinner commented Jun 13, 2022

LGTM, great debugging :D

My PR #93776 should detect such bug, to make the debug way more straightforward ;-)

@jaraco
Copy link
Member

@jaraco jaraco commented Jun 13, 2022

This probably needs to be ported to importlib_resources.

@jaraco jaraco mentioned this pull request Jun 13, 2022
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka

@warsaw warsaw

Assignees
No one assigned
Labels
expert-importlib
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@vstinner @carlbordum @miss-islington @bedevere-bot @jaraco @warsaw @serhiy-storchaka @AA-Turner