Skip to content

bpo-40671: Prepare _hashlib for PEP 489 (GH-20180) #20180

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.

Sign up for GitHub

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
Merged
merged 1 commit into from
May 25, 2020
Merged

bpo-40671: Prepare _hashlib for PEP 489 (GH-20180) #20180

merged 1 commit into from
May 25, 2020

Conversation

tiran
Copy link
Member

@tiran tiran commented May 18, 2020 •
edited by miss-islington
Loading

Prepare _hashlib module for PEP 489 module initialization and use PyModule_AddType.

Signed-off-by: Christian Heimes [email protected]

https://bugs.python.org/issue40671

Automerge-Triggered-By: @tiran

vstinner
vstinner reviewed May 18, 2020
View reviewed changes
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 hold this PR until master becomes Python 3.10. It's too late for 3.9, and I don't think that it's worth it to modify 3.9. This PR is mostly useful for subinterpreters which remains an experimental feature (especially with per-interpreter GIL).

shihai1991
shihai1991 reviewed May 18, 2020
View reviewed changes
shihai1991
shihai1991 reviewed May 18, 2020
View reviewed changes
shihai1991
shihai1991 reviewed May 18, 2020
View reviewed changes
vstinner
vstinner reviewed May 18, 2020
View reviewed changes
@vstinner
Copy link
Member

Note: I'm happy to see my gc module debug enhancements working as expected :-)

Modules/gcmodule.c:441: visit_decref: Assertion "!_PyObject_IsFreed(op)" failed
Enable tracemalloc to get the memory block allocation traceback

object address  : 0x7fe5191767d0
object refcount : 1
object type     : 0x818280
object type name: dict
object repr     :

Sadly, _PyObject_Dump() fails to render a dictionary which contains a "freed" item. That's another possible enhancement ;-)

@tiran tiran added the needs backport to 3.9 only security fixes label May 19, 2020
@tiran
Copy link
Member Author

tiran commented May 19, 2020

I have rebased the PR on master and submitted a new build. The Ubuntu builder was acting up.

@tiran tiran requested a review from vstinner May 19, 2020 10:54
@tiran
Copy link
Member Author

tiran commented May 19, 2020

Please hold this PR until master becomes Python 3.10. It's too late for 3.9, and I don't think that it's worth it to modify 3.9. This PR is mostly useful for subinterpreters which remains an experimental feature (especially with per-interpreter GIL).

This is a minor and internal change. Other modules are already using the new API. I prefer to backport the change to 3.9 to reduce maintenance my burden.

@vstinner
Copy link
Member

This is a minor and internal change.

It's a new feature: it becomes possible to have two separated instances of the module. There is a risk of regression.

@tiran
bpo-40671: Prepare _hashlib for PEP 489
6aaa162 
Prepare ``_hashlib`` module for PEP 489 module initialization
and use ``PyModule_AddType``.

Signed-off-by: Christian Heimes <[email protected]>
@tiran
Copy link
Member Author

tiran commented May 20, 2020

This is a minor and internal change.

It's a new feature: it becomes possible to have two separated instances of the module. There is a risk of regression.

Sounds plausible. I have changed the PR so salvage as much of the changes without breaking behavior.

@tiran tiran changed the title bpo-40671: PEP 489 for _hashlib bpo-40671: Prepare _hashlib for PEP 489 May 20, 2020
@tiran
Copy link
Member Author

tiran commented May 20, 2020

@shihai1991 could you take another look, please? I modified the PR to prepare the hashlib module for PEP 489 but does not switch to the new API. I'd like to keep the code in 3.9 and master as close as possible to reduce my maintenance overhead.

shihai1991
shihai1991 reviewed May 21, 2020
View reviewed changes
Copy link
Member

@shihai1991 shihai1991 left a comment

Choose a reason for hiding this comment

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

LGTM.

Modules/_hashopenssl.c
PyMODINIT_FUNC
PyInit__hashlib(void)
{
PyObject *m = PyState_FindModule(&_hashlibmodule);
Copy link
Member

Choose a reason for hiding this comment

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

nitpick from victor: using mod or module would be more exact ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

$ grep -R "\sPyObject \*m[,;]" Modules/*.c | wc -l
40
$ grep -R "\sPyObject \*module[,;]" Modules/*.c | wc -l
8

m is common.

Copy link
Member

Choose a reason for hiding this comment

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

Hm, we will update all m vars when we port extension module to pep489 ; )

@tiran tiran mentioned this pull request May 21, 2020
Merged
@csabella
Copy link
Contributor

Since master is now 3.10, is this just waiting for someone to hit the approve button?

@tiran
Copy link
Member Author

tiran commented May 23, 2020

Since master is now 3.10, is this just waiting for someone to hit the approve button?

Yes, I'm waiting for approval.

Please note that I modified the PR so that it does not change behavior at all so it can land in 3.9. I'll do another PR on top that will enable PR 489 behavior for 3.10 only.

@tiran tiran changed the title bpo-40671: Prepare _hashlib for PEP 489 bpo-40671: Prepare _hashlib for PEP 489 (GH-20180) May 25, 2020
@tiran tiran merged commit 20c22db into python:master May 25, 2020
@tiran tiran deleted the bpo40671 branch May 25, 2020 08:44
@miss-islington
Copy link
Contributor

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 25, 2020
@tiran @miss-islington
bpo-40671: Prepare _hashlib for PEP 489 (pythonGH-20180)
f61e9c3 
(cherry picked from commit 20c22db)

Co-authored-by: Christian Heimes <[email protected]>
@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label May 25, 2020
@bedevere-bot
Copy link

GH-20378 is a backport of this pull request to the 3.9 branch.

@tiran tiran added the needs backport to 3.9 only security fixes label May 25, 2020
@miss-islington
Copy link
Contributor

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 25, 2020
@tiran @miss-islington
bpo-40671: Prepare _hashlib for PEP 489 (pythonGH-20180)
33f66e4 
(cherry picked from commit 20c22db)

Co-authored-by: Christian Heimes <[email protected]>
@bedevere-bot
Copy link

GH-20381 is a backport of this pull request to the 3.9 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.9 only security fixes label May 25, 2020
miss-islington added a commit that referenced this pull request May 25, 2020
@miss-islington @tiran
bpo-40671: Prepare _hashlib for PEP 489 (GH-20180)
1fe1a14 
(cherry picked from commit 20c22db)

Co-authored-by: Christian Heimes <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shihai1991 shihai1991 shihai1991 left review comments

@vstinner vstinner Awaiting requested review from vstinner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@tiran @vstinner @csabella @miss-islington @bedevere-bot @shihai1991 @the-knights-who-say-ni