The Wayback Machine - https://web.archive.org/web/20240110155336/https://github.com/python/cpython/pull/113813
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

gh-113781: Silence AttributeError in warning module during Python finalization #113813

Merged

Conversation

serhiy-storchaka
Copy link
Member

@serhiy-storchaka serhiy-storchaka commented Jan 8, 2024

The tracemalloc module can already be cleared.

…on finalization

The tracemalloc module can already be cleared.
@serhiy-storchaka
Copy link
Member Author

I was not able to write a simple test for this.

@serhiy-storchaka serhiy-storchaka added needs backport to 3.11 bug and security fixes needs backport to 3.12 bug and security fixes labels Jan 8, 2024
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Thanks! A thought.

Lib/warnings.py Outdated
tb = tracemalloc.get_object_traceback(msg.source)
except Exception:
# When a warning is logged during Python shutdown, tracemalloc
# and the import machinery don't work anymore
tracing = False
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be True? Like above at line 61. And note below at line 88/89, the "enable tracemalloc" message is appended if tracing is false.

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought that except Exception was added because the tracemalloc module was optional. In that case it is reasonable that we should not suggest to use the tracemalloc module.

But in fact, it most likely fails because the import system is already broken at Python shutdown, so no new modules can be imported. And if we are in the else block, then either the import system works, or tracemalloc was imported earlier. Can we suggest to enable it? Who knows. I initially thought that we can, running Python with -X tracemalloc seems worked in these examples. But I failed to write tests that do not involve running tests via unittest (or even libregrtest), so I am now not sure that it always works. So perhaps it is safer to set tracing = True here.

Copy link
Member

Choose a reason for hiding this comment

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

The tracing variable is a bit of a misnomer -- it's just used to suppress the "Enable tracemalloc" message. Maybe reverse the sense and rename it to "suggest_tracemalloc"? Then we'd get

# If we're already tracing, don't suggest to enable tracemalloc
suggest_tracemalloc = not tracemalloc.is_tracing()

The idea being that if we're already tracing the suggestion is redundant. (See the original PR gh-10486 where it was introduced.)

Lib/warnings.py Outdated Show resolved Hide resolved
Copy link
Member

@gvanrossum gvanrossum left a comment

Choose a reason for hiding this comment

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

Go for it!

@serhiy-storchaka serhiy-storchaka merged commit 0297418 into python:main Jan 9, 2024
31 checks passed
@serhiy-storchaka serhiy-storchaka deleted the warnings-tracemalloc-shutdown branch January 9, 2024 19:44
@miss-islington-app
Copy link

Thanks @serhiy-storchaka for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 9, 2024
…on finalization (pythonGH-113813)

The tracemalloc module can already be cleared.
(cherry picked from commit 0297418)

Co-authored-by: Serhiy Storchaka <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Jan 9, 2024

GH-113873 is a backport of this pull request to the 3.12 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 9, 2024
…on finalization (pythonGH-113813)

The tracemalloc module can already be cleared.
(cherry picked from commit 0297418)

Co-authored-by: Serhiy Storchaka <[email protected]>
@bedevere-app bedevere-app bot removed the needs backport to 3.12 bug and security fixes label Jan 9, 2024
@bedevere-app
Copy link

bedevere-app bot commented Jan 9, 2024

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

@bedevere-app bedevere-app bot removed the needs backport to 3.11 bug and security fixes label Jan 9, 2024
serhiy-storchaka added a commit that referenced this pull request Jan 9, 2024
…hon finalization (GH-113813) (GH-113873)

The tracemalloc module can already be cleared.
(cherry picked from commit 0297418)

Co-authored-by: Serhiy Storchaka <[email protected]>
serhiy-storchaka added a commit that referenced this pull request Jan 9, 2024
…hon finalization (GH-113813) (GH-113874)

The tracemalloc module can already be cleared.
(cherry picked from commit 0297418)

Co-authored-by: Serhiy Storchaka <[email protected]>
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.

None yet

2 participants