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
Clarify that every thread has its own default context in contextvars #99246
Conversation
Doc/library/contextvars.rst
Outdated
:class:`~contextvars.Context`. This means that a | ||
:class:`~contextvars.ContextVar` can generally be used as a drop-in | ||
replacement for a :func:`threading.local` variable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that saying that ContextVar
can be used as a drop-in for threading.local
might bring people to blindly replace threading.local
with ContextVar
, but care is needed. Some things (mostly C stuff) are just bound to OS threads, so threading.local
is what you want for them, while with ContextVar
there can be many contexts per OS thread (e.g. asyncio tasks).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately I don't think I follow your argument, Can you suggest a better wording for this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the first sentence provides good information, although a couple notes on the wording:
-
"By default" -- is there a way for a thread to not have a unique context? Maybe with the C API? PEP-567 says "Context.run() raises a RuntimeError when called on the same context object from more than one OS thread" so it shouldn't be possible to "share" a
Context
. If so, I'd remove "By default". -
"default Context" -- maybe instead say "root Context" or "initial Context" or "top-level Context"?
Regarding the second sentence, what is the motivation for having a note about it? Is the idea to encourage people to switch to ContextVar
when it's the better choice?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Regarding the second sentence, what is the motivation for having a note about it?
The motivation is to clarify the following sentence:
Context managers that have state should use Context Variables
instead of :func:threading.local()
to prevent their state from
bleeding to other code unexpectedly, when used in concurrent code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My suggestion is to replacie the note+paragraph with the following:
Every thread has a unique current :class:`~contextvars.Context`.
If you're currently using :func:`threading.local()` to store contextual information,
such as context manager state or per-request state in web frameworks,
consider using Context Variables instead. This will prevent the state from
bleeding to other contexts unexpectedly, when used in concurrent code
that is running in the same thread.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't agree. I want to highlight more that every thread has is own context.
Thanks for the feedback I will take it into account into the final version
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is a later line "Context
objects hold strong references to context variables which prevents context variables from being properly garbage collected." Is this also true for threading.local()
? And if not, is that potentially a problem for using "drop-in replacement"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's also true for thread locals, they also hold strong references to what they contain. Is just that they are less tricky to leak because they don't have the layers separating the key and the container.
Going from thread local to context variable (as long as you place them at top level as the doc say) will not create ownership problems in general
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In any case I will use a different wording as I just want to focus on the per thread default guarantee here, not on the equivalence between this and thread locals.
Signed-off-by: Pablo Galindo <pablogsal@gmail.com>
Co-authored-by: Stanley <46876382+slateny@users.noreply.github.com>
Thanks @pablogsal for the PR |
Sorry @pablogsal, I had trouble checking out the |
GH-100365 is a backport of this pull request to the 3.10 branch. |
GH-100366 is a backport of this pull request to the 3.9 branch. |
…ythonGH-99246) (cherry picked from commit cb60b61) Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>
…ythonGH-99246) (cherry picked from commit cb60b61) Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>
Thanks @pablogsal for the PR |
GH-100367 is a backport of this pull request to the 3.11 branch. |
…ythonGH-99246) (cherry picked from commit cb60b61) Co-authored-by: Pablo Galindo Salgado <Pablogsal@gmail.com>
No description provided.