Skip to content

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

Merged
serhiy-storchaka merged 3 commits into
python:mainfrom
serhiy-storchaka:warnings-tracemalloc-shutdown
Jan 9, 2024
Merged

gh-113781: Silence AttributeError in warning module during Python finalization#113813
serhiy-storchaka merged 3 commits into
python:mainfrom
serhiy-storchaka:warnings-tracemalloc-shutdown

Conversation

@serhiy-storchaka

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

Copy link
Copy Markdown
Member

The tracemalloc module can already be cleared.

…on finalization

The tracemalloc module can already be cleared.
@serhiy-storchaka

Copy link
Copy Markdown
Member Author

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

@serhiy-storchaka serhiy-storchaka added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Jan 8, 2024

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks! A thought.

Comment thread Lib/warnings.py Outdated
except Exception:
# When a warning is logged during Python shutdown, tracemalloc
# and the import machinery don't work anymore
tracing = False

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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.)

Comment thread Lib/warnings.py Outdated

@gvanrossum gvanrossum left a comment

Copy link
Copy Markdown
Member

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
@serhiy-storchaka
serhiy-storchaka deleted the warnings-tracemalloc-shutdown branch January 9, 2024 19:44
@miss-islington-app

Copy link
Copy Markdown

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 <storchaka@gmail.com>
@bedevere-app

bedevere-app Bot commented Jan 9, 2024

Copy link
Copy Markdown

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 <storchaka@gmail.com>
@bedevere-app bedevere-app Bot removed the needs backport to 3.12 only security fixes label Jan 9, 2024
@bedevere-app

bedevere-app Bot commented Jan 9, 2024

Copy link
Copy Markdown

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 only 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 <storchaka@gmail.com>
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 <storchaka@gmail.com>
kulikjak pushed a commit to kulikjak/cpython that referenced this pull request Jan 22, 2024
…on finalization (pythonGH-113813)

The tracemalloc module can already be cleared.
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
…on finalization (pythonGH-113813)

The tracemalloc module can already be cleared.
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
…on finalization (pythonGH-113813)

The tracemalloc module can already be cleared.
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.

2 participants