Skip to content

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

Merged
tiran merged 1 commit into
python:masterfrom
tiran:bpo40671
May 25, 2020
Merged

bpo-40671: Prepare _hashlib for PEP 489 (GH-20180)#20180
tiran merged 1 commit into
python:masterfrom
tiran:bpo40671

Conversation

@tiran

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

Copy link
Copy Markdown
Member

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

Signed-off-by: Christian Heimes christian@python.org

https://bugs.python.org/issue40671

Automerge-Triggered-By: @tiran

vstinner
vstinner reviewed May 18, 2020
View reviewed changes

@vstinner vstinner 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.

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).

Comment thread Modules/_hashopenssl.c Outdated
Comment thread Modules/_hashopenssl.c Outdated
Comment thread Modules/_hashopenssl.c Outdated
shihai1991
shihai1991 reviewed May 18, 2020
View reviewed changes
Comment thread Misc/NEWS.d/next/Library/2020-05-18-15-26-31.bpo-40671.NeZ9Cy.rst Outdated
shihai1991
shihai1991 reviewed May 18, 2020
View reviewed changes
Comment thread Modules/_hashopenssl.c Outdated
shihai1991
shihai1991 reviewed May 18, 2020
View reviewed changes
Comment thread Modules/_hashopenssl.c Outdated
vstinner
vstinner reviewed May 18, 2020
View reviewed changes
Comment thread Modules/_hashopenssl.c Outdated
@vstinner

vstinner commented May 18, 2020

Copy link
Copy Markdown
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 commented May 19, 2020

Copy link
Copy Markdown
Member Author

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

tiran commented May 19, 2020

Copy link
Copy Markdown
Member Author

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

vstinner commented May 20, 2020

Copy link
Copy Markdown
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 <christian@python.org>
@tiran

tiran commented May 20, 2020

Copy link
Copy Markdown
Member Author

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

tiran commented May 20, 2020

Copy link
Copy Markdown
Member Author

@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

@shihai1991 shihai1991 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.

LGTM.

Comment thread Modules/_hashopenssl.c
PyMODINIT_FUNC
PyInit__hashlib(void)
{
PyObject *m = PyState_FindModule(&_hashlibmodule);

@shihai1991 shihai1991 May 21, 2020

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.

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

@tiran tiran May 21, 2020

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. 

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

m is common.

@shihai1991 shihai1991 May 21, 2020

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.

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

csabella commented May 23, 2020

Copy link
Copy Markdown
Contributor

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

@tiran

tiran commented May 23, 2020

Copy link
Copy Markdown
Member Author

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

miss-islington commented May 25, 2020

Copy link
Copy Markdown
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 <christian@python.org>
@bedevere-bot

bedevere-bot commented May 25, 2020

Copy link
Copy Markdown

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

@miss-islington

miss-islington commented May 25, 2020

Copy link
Copy Markdown
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 <christian@python.org>
@bedevere-bot

bedevere-bot commented May 25, 2020

Copy link
Copy Markdown

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

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 <christian@python.org>
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