Skip to content

bpo-1635741: Enhance _datetime error handling #23139

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

Merged
merged 4 commits into from
Nov 20, 2020

Conversation

koubaa
Copy link
Contributor

@koubaa koubaa commented Nov 4, 2020

This helps prepare for multiphase init (PEP 489)

https://bugs.python.org/issue1635741

@koubaa
Copy link
Contributor Author

koubaa commented Nov 4, 2020

@vstinner please review

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

Please mention the modified module name in the PR title.

I suggest to canghe the PR title to "Enhance _datetime error handling" or "Refactor _datetime error handling", since this change is not directly related to multi-phase init. I'm not sure that we can soon convert this module to the multi-phase init because of the PyCapsule C API.

@vstinner
Copy link
Member

vstinner commented Nov 4, 2020

You wrote the wrong bpo number in the PR title.

@pganssle
Copy link
Member

pganssle commented Nov 4, 2020

@koubaa Can you provide a TL;DR of why this change is useful or necessary?

@vstinner
Copy link
Member

vstinner commented Nov 4, 2020

@koubaa Can you provide a TL;DR of why this change is useful or necessary?

https://bugs.python.org/issue1635741 describes the overall goal: better embed Python in application. One side of the issue is to ensure that an extension module releases all its resources at Python exit.

@asvetlov asvetlov changed the title bpo-1536741: refactoring before multi-phase init bpo-1536741: refactoring datetime before multi-phase init Nov 4, 2020
@koubaa koubaa changed the title bpo-1536741: refactoring datetime before multi-phase init bpo-1635741: Enhance _datetime error handling Nov 7, 2020
@koubaa koubaa force-pushed the bpo-1635741-datetime branch from 8910cc2 to 04889ea Compare November 7, 2020 21:37

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@koubaa koubaa force-pushed the bpo-1635741-datetime branch from 04889ea to ae49eaf Compare November 15, 2020 17:55
Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

Be careful with preprocessor traps :-( https://gcc.gnu.org/onlinedocs/cpp/Macro-Pitfalls.html

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@koubaa
Copy link
Contributor Author

koubaa commented Nov 20, 2020

Be careful with preprocessor traps :-( https://gcc.gnu.org/onlinedocs/cpp/Macro-Pitfalls.html

Thanks again for sharing this, I easily forget them.

@vstinner
Copy link
Member

Thanks again for sharing this, I easily forget them.

Oh, me tee! I got exactly the same issue than you a few days ago :-D

@vstinner vstinner merged commit 2db8e35 into python:master Nov 20, 2020
@vstinner
Copy link
Member

Thanks, I merged your PR.

I checked manually for refleak using the following test run with "./python -m test (...) -R 3:3":

    def test_leak(self):
        support.run_in_subinterp("import _datetime")

I didn't notice any refleak.

@vstinner
Copy link
Member

Hum, I'm not sure that my manual test is relevant, since _datetime doesn't use multiphase init yet. But it leaks, I don't expect issues with Refleaks buildbots.

adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants