Skip to content
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

gh-119344: Make critical section API public #119353

Merged
merged 12 commits into from
Jun 21, 2024

Conversation

colesbury
Copy link
Contributor

@colesbury colesbury commented May 21, 2024

This makes the following macros public as part of the non-limited C-API for locking a single object or two objects at once.

  • Py_BEGIN_CRITICAL_SECTION(op) / Py_END_CRITICAL_SECTION()
  • Py_BEGIN_CRITICAL_SECTION2(a, b) / Py_END_CRITICAL_SECTION2()

The following supporting functions and types are also public for use by bindings from other languages:

  • PyAPI_FUNC(void) PyCriticalSection_Begin(PyCriticalSection *c, PyObject *op)
  • PyAPI_FUNC(void) PyCriticalSection_End(PyCriticalSection *c)
  • PyAPI_FUNC(void) PyCriticalSection_Begin2(PyCriticalSection2 *c, PyObject *a, PyObject *b)
  • PyAPI_FUNC(void) PyCriticalSection_End2(PyCriticalSection2 *c)
  • PyCriticalSection (struct contents private)
  • PyCriticalSection2 (struct contents private)

Note that the more esoteric APIs are still internal-only as part of pycore_critical_section.h.

See also C-API WG Issue:

Docs:

@colesbury colesbury marked this pull request as ready for review June 20, 2024 23:10
@colesbury colesbury added the needs backport to 3.13 bugs and security fixes label Jun 20, 2024
@colesbury colesbury force-pushed the gh-119344-critical-section-api branch from fb1579a to c61ba72 Compare June 20, 2024 23:53
@colesbury colesbury requested review from vstinner and encukou and removed request for hugovk and ezio-melotti June 20, 2024 23:53
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.

Did this API got an exception to be added to Python 3.13?

Doc/c-api/init.rst Outdated Show resolved Hide resolved
// Python releases without a deprecation period.
struct PyCriticalSection {
// Tagged pointer to an outer active critical section (or 0).
uintptr_t _prev;
Copy link
Member

Choose a reason for hiding this comment

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

Would it be possible to add "cs_" in the name? It helps refactoring and to navigate in the code.

  • _prev => _cs_prev
  • _mutex => _cs_mutex

Same suggestion for PyCriticalSection2.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've renamed the fields

# error "this header file must not be included directly"
#endif

// Python critical sections
Copy link
Member

Choose a reason for hiding this comment

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

Maybe move this doc to Doc/c-api/init.rst, or merge it with Doc/c-api/init.rst?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the file documentation is still useful even with the public Doc/c-api/init.rst docs. The file docs talk more about specific internals like PyThreadState.critical_section.

Copy link
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

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

Looks good, thank you! I have a few suggestions for the docs:

Doc/c-api/init.rst Outdated Show resolved Hide resolved
Doc/c-api/init.rst Outdated Show resolved Hide resolved
Doc/c-api/init.rst Show resolved Hide resolved
@@ -2195,3 +2195,100 @@ The C-API provides a basic mutual exclusion lock.
issue a fatal error.

.. versionadded:: 3.13

Python Critical Section API
Copy link
Contributor

Choose a reason for hiding this comment

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

Mabye add a label for easy referencing in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a label

Doc/c-api/init.rst Outdated Show resolved Hide resolved
Co-authored-by: Petr Viktorin <encukou@gmail.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Victor Stinner <vstinner@python.org>
@colesbury
Copy link
Contributor Author

@Yhg1s - this PR makes the critical section API public. Is it acceptable to include this in 3.13?

There's a bunch of refactoring to make the API public, but no new functionality.

@Yhg1s
Copy link
Member

Yhg1s commented Jun 21, 2024

Yes, this can go into 3.13 if it gets into beta 3. (How many more APIs do you think we'll need for 3.13? :P)

@colesbury
Copy link
Contributor Author

This is the last one, I promise!

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.

LGTM, thanks for the update.

@colesbury colesbury merged commit 8f17d69 into python:main Jun 21, 2024
38 checks passed
@miss-islington-app
Copy link

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

@colesbury colesbury deleted the gh-119344-critical-section-api branch June 21, 2024 19:50
@miss-islington-app
Copy link

Sorry, @colesbury, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 8f17d69b7bc906e8407095317842cc0fd52cd84a 3.13

colesbury added a commit to colesbury/cpython that referenced this pull request Jun 21, 2024
…9353)

This makes the following macros public as part of the non-limited C-API for
locking a single object or two objects at once.

* `Py_BEGIN_CRITICAL_SECTION(op)` / `Py_END_CRITICAL_SECTION()`
* `Py_BEGIN_CRITICAL_SECTION2(a, b)` / `Py_END_CRITICAL_SECTION2()`

The supporting functions and structs used by the macros are also exposed for
cases where C macros are not available.
(cherry picked from commit 8f17d69)

Co-authored-by: Sam Gross <colesbury@gmail.com>
@bedevere-app
Copy link

bedevere-app bot commented Jun 21, 2024

GH-120856 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jun 21, 2024
colesbury added a commit that referenced this pull request Jun 21, 2024
This makes the following macros public as part of the non-limited C-API for
locking a single object or two objects at once.

* `Py_BEGIN_CRITICAL_SECTION(op)` / `Py_END_CRITICAL_SECTION()`
* `Py_BEGIN_CRITICAL_SECTION2(a, b)` / `Py_END_CRITICAL_SECTION2()`

The supporting functions and structs used by the macros are also exposed for
cases where C macros are not available.
(cherry picked from commit 8f17d69)
mrahtz pushed a commit to mrahtz/cpython that referenced this pull request Jun 30, 2024
This makes the following macros public as part of the non-limited C-API for
locking a single object or two objects at once.

* `Py_BEGIN_CRITICAL_SECTION(op)` / `Py_END_CRITICAL_SECTION()`
* `Py_BEGIN_CRITICAL_SECTION2(a, b)` / `Py_END_CRITICAL_SECTION2()`

The supporting functions and structs used by the macros are also exposed for
cases where C macros are not available.
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
This makes the following macros public as part of the non-limited C-API for
locking a single object or two objects at once.

* `Py_BEGIN_CRITICAL_SECTION(op)` / `Py_END_CRITICAL_SECTION()`
* `Py_BEGIN_CRITICAL_SECTION2(a, b)` / `Py_END_CRITICAL_SECTION2()`

The supporting functions and structs used by the macros are also exposed for
cases where C macros are not available.
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
This makes the following macros public as part of the non-limited C-API for
locking a single object or two objects at once.

* `Py_BEGIN_CRITICAL_SECTION(op)` / `Py_END_CRITICAL_SECTION()`
* `Py_BEGIN_CRITICAL_SECTION2(a, b)` / `Py_END_CRITICAL_SECTION2()`

The supporting functions and structs used by the macros are also exposed for
cases where C macros are not available.
@colesbury colesbury removed their assignment Jul 19, 2024
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.

6 participants