Skip to content

gh-138098: Clarify strong references in PyDict_Next docs for free-threaded build #138106

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 8 commits into from
Aug 25, 2025

Conversation

PrinceNaroliya
Copy link
Contributor

@PrinceNaroliya PrinceNaroliya commented Aug 24, 2025

This PR updates the documentation of :c:func:PyDict_Next to clarify the
handling of references in the free-threaded build.

Currently, the docs mention that PyDict_Next is not safe in the free-threaded
build and suggest using a critical section. However, the returned pkey and
pvalue are borrowed references, which are only valid while the critical
section is held. If they need to be accessed outside of that critical section,
strong references must be created.

This PR adds a .. note:: block explaining this behavior explicitly, and
suggests using :c:func:Py_NewRef to obtain strong references.

Rationale

  • Makes the documentation safer and clearer for users of the free-threaded
    build.
  • Resolves issue gh-138098.
  • Matches the behavior noted in gh-119438.

Example of new docs rendering

Screenshot 2025-08-24 125023

Thanks!


📚 Documentation preview 📚: https://cpython-previews--138106.org.readthedocs.build/en/138106/c-api/dict.html#c.PyDict_Next

Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

Looks mostly fine, we just need to add links to the terms.

@PrinceNaroliya
Copy link
Contributor Author

Thanks for the helpful reviews!
I’ve applied the suggested changes:

  • Added glossary links for free threaded, borrowed reference, and strong reference
  • Clarified the wording about usage outside/suspended critical sections
  • Mentioned creating strong references with :c:func:Py_NewRef

Please let me know if anything else needs to be updated ✅

@eendebakpt
Copy link
Contributor

The text itself is good now. But the position in the documentation is a bit odd. The note is separated from the other text on free threading. For that reason, I would move the note down a bit.

@PrinceNaroliya
Copy link
Contributor Author

PrinceNaroliya commented Aug 25, 2025

"I have tried adding the .. note:: directive in Sphinx multiple times.
When I place the note at the top, it renders correctly as a highlighted note box.
However, when I try to paste the same note at the bottom, it only appears as normal text and does not display as a note.
I have checked indentation, blank lines, and code-block formatting, but the problem persists.
It seems to be an issue with how Sphinx handles notes at the bottom rather than a syntax error.

@ZeroIntensity
Copy link
Member

It works for me:

image

@PrinceNaroliya
Copy link
Contributor Author

I have fixed the issue, everything seems correct now. Previously the note was not displaying properly at the top, but now it appears correctly at the bottom

Copy link
Member

@ZeroIntensity ZeroIntensity left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me.

@eendebakpt As the issue author, are you happy with this? If so, I'll merge this.

Copy link
Contributor

@eendebakpt eendebakpt 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 to me. Thanks @PrinceNaroliya

@ZeroIntensity ZeroIntensity added the needs backport to 3.14 bugs and security fixes label Aug 25, 2025
@PrinceNaroliya
Copy link
Contributor Author

Thanks for the helpful reviews! I’m happy with the changes and everything looks good now.

@ZeroIntensity ZeroIntensity merged commit 9ee0214 into python:main Aug 25, 2025
33 checks passed
@github-project-automation github-project-automation bot moved this from Todo to Done in Docs PRs Aug 25, 2025
@miss-islington-app
Copy link

Thanks @PrinceNaroliya for the PR, and @ZeroIntensity for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Aug 25, 2025
…he free-threaded build (pythonGH-138106)

(cherry picked from commit 9ee0214b5dd982ac9fbe18dcce0e8787456e29af)

Co-authored-by: PrinceNaroliya <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Aug 25, 2025

GH-138141 is a backport of this pull request to the 3.14 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.14 bugs and security fixes label Aug 25, 2025
@PrinceNaroliya PrinceNaroliya deleted the doc-pydict-next-strong-ref branch August 25, 2025 15:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation in the Doc dir skip news
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants