gh-111916: Make hashlib related modules thread-safe without the GIL by tomasr8 · Pull Request #111981 · python/cpython · GitHub
Skip to content

gh-111916: Make hashlib related modules thread-safe without the GIL - #111981

Merged
gpshead merged 11 commits into
python:mainfrom
tomasr8:hashlib
Nov 15, 2023
Merged

gh-111916: Make hashlib related modules thread-safe without the GIL#111981
gpshead merged 11 commits into
python:mainfrom
tomasr8:hashlib

Conversation

@tomasr8

@tomasr8 tomasr8 commented Nov 11, 2023 •
edited by gpshead
Loading

Copy link
Copy Markdown
Member

Still a work in progress but I'd be happy for feedback ;)

I'm not entirely sure what to do about HASHLIB_GIL_MINSIZE. IIUC the original code avoids releasing the GIL and getting a separate lock when the update buffer is short enough. I'm guessing that in the nogil version, we'll have to lock the mutex regardless of the length (i.e. essentially getting rid of the macro)? Is that correct?

As a side note, I'm failing to build this PR with the GIL enabled. Running ./configure --with-pydebug and make -s -j2, I'm getting this error:

In file included from ./Modules/hashlib.h:7,
                 from ./Modules/md5module.c:29:
./Include/internal/pycore_lock.h: In function ‘PyMutex_LockFast’:
./Include/internal/pycore_lock.h:60:12: error: implicit declaration of function ‘_Py_atomic_compare_exchange_uint8’; did you mean ‘__atomic_compare_exchange_n’? [-Werror=implicit-function-declaration]
   60 |     return _Py_atomic_compare_exchange_uint8(lock_bits, &expected, _Py_LOCKED);

Even if I add #include "pyatomic.h" to pycore_lock.h I get the same error.. any idea why that is?

Fixes #111916

@bedevere-app bedevere-app Bot mentioned this pull request Nov 11, 2023
Closed
colesbury
colesbury reviewed Nov 13, 2023
View reviewed changes

@colesbury colesbury left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The build errors you're seeing are due to md5module.c being compiled with the limited API, which means internal-only functions like PyMutex_Lock are not available. You should remove these lines from md5module.c:

#ifndef _MSC_VER
#include "pyconfig.h"   // Py_NOGIL
#endif
#ifndef Py_NOGIL
// Need limited C API version 3.12 for Py_MOD_PER_INTERPRETER_GIL_SUPPORTED
#define Py_LIMITED_API 0x030c0000
#endif

And instead add:

#ifndef Py_BUILD_CORE_BUILTIN
#  define Py_BUILD_CORE_MODULE 1
#endif

Comment thread Modules/_blake2/blake2s_impl.c Outdated
Comment thread Modules/_blake2/blake2b_impl.c Outdated
Comment thread Modules/_hashopenssl.c Outdated
Comment thread Modules/_hashopenssl.c Outdated
Comment thread Modules/_hashopenssl.c Outdated
Comment thread Modules/hashlib.h Outdated
Comment thread Modules/hashlib.h Outdated
Comment thread Modules/sha3module.c Outdated
@colesbury

colesbury commented Nov 13, 2023

Copy link
Copy Markdown
Contributor

@tomasr8
tomasr8 marked this pull request as ready for review November 14, 2023 21:13
@tomasr8
tomasr8 requested a review from tiran as a code owner November 14, 2023 21:13
@colesbury

colesbury commented Nov 14, 2023

Copy link
Copy Markdown
Contributor

It looks like some of the generated parts of Modules/_blake2/blake2s_impl.c are out of date. You'll need to re-run Argument Clinic. I think python3 Tools/clinic/clinic.py --make --exclude Lib/test/clinic.test.c --srcdir . should do the trick.

@tomasr8

tomasr8 commented Nov 15, 2023

Copy link
Copy Markdown
Member Author

Thanks for the thorough review! I reran AC as you suggested, so hopefully it should be good this time

@colesbury
colesbury requested a review from gpshead November 15, 2023 18:13
colesbury
colesbury approved these changes Nov 15, 2023
View reviewed changes

@colesbury colesbury left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks, @tomasr8! This LGTM.

@gpshead - would you be able to review this PR? (I saw your name on the git log for these files.)

@gpshead gpshead self-assigned this Nov 15, 2023
gpshead
gpshead reviewed Nov 15, 2023
View reviewed changes
Comment thread Modules/md5module.c Outdated
gpshead
gpshead approved these changes Nov 15, 2023
View reviewed changes

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

one minor style nit, overall looks great. thanks!

@gpshead

gpshead commented Nov 15, 2023

Copy link
Copy Markdown
Member

@gpshead
gpshead enabled auto-merge (squash) November 15, 2023 23:37
@gpshead
gpshead merged commit a646560 into python:main Nov 15, 2023
@tomasr8
tomasr8 deleted the hashlib branch November 16, 2023 07:05
aisk pushed a commit to aisk/cpython that referenced this pull request Feb 11, 2024
@tomasr8 @aisk
pythongh-111916: Make hashlib related modules thread-safe without the…
fcc7a7e 
… GIL (python#111981)

Always use an individual lock on hash objects when in free-threaded builds.

Fixes python#111916
Glyphack pushed a commit to Glyphack/cpython that referenced this pull request Sep 2, 2024
@tomasr8 @Glyphack
pythongh-111916: Make hashlib related modules thread-safe without the…
a0c3bb8 
… GIL (python#111981)

Always use an individual lock on hash objects when in free-threaded builds.

Fixes python#111916
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpshead gpshead gpshead approved these changes
@colesbury colesbury colesbury approved these changes
@tiran tiran Awaiting requested review from tiran

Assignees

@gpshead gpshead

Labels

topic-free-threading

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Make hashlib related modules thread-safe without the GIL

3 participants

@tomasr8 @colesbury @gpshead