gh-124872: Replace enter/exit events with "switched" #124776
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This was referenced Sep 30, 2024
rhansen
force-pushed
the
gh-119333-context-switched
branch
4 times, most recently
from
8242dce to
7606b67
Compare
rhansen
changed the title
rhansen
force-pushed
the
gh-119333-context-switched
branch
3 times, most recently
from
e68cb7f to
d04d9c0
Compare
rhansen
force-pushed
the
gh-119333-context-switched
branch
from
d04d9c0 to
f3f17b3
Compare
picnixz
reviewed
Oct 12, 2024
rhansen
force-pushed
the
gh-119333-context-switched
branch
from
f3f17b3 to
b4cff77
Compare
Users want to know when the current context switches to a different context object. Right now this happens when and only when a context is entered or exited, so the enter and exit events are synonymous with "switched". However, if the changes proposed for pythongh-99633 are implemented, the current context will also switch for reasons other than context enter or exit. Since users actually care about context switches and not enter or exit, replace the enter and exit events with a single switched event. The former exit event was emitted just before exiting the context. The new switched event is emitted after the context is exited to match the semantics users expect of an event with a past-tense name. If users need the ability to clean up before the switch takes effect, another event type can be added in the future. It is not added here because YAGNI. I skipped 0 in the enum as a matter of practice. Skipping 0 makes it easier to troubleshoot when code forgets to set zeroed memory, and it aligns with best practices for other tools (e.g., https://protobuf.dev/programming-guides/dos-donts/#unspecified-enum).
rhansen
force-pushed
the
gh-119333-context-switched
branch
from
b4cff77 to
d898525
Compare
ZeroIntensity
approved these changes
Oct 12, 2024
|
This is sort of reviewed already, per discussion in the original issue, so I'm approving it. |
1st1
approved these changes
Oct 14, 2024
1st1
reviewed
Oct 14, 2024
| case Py_CONTEXT_EVENT_EXIT: | ||
| return "Py_CONTEXT_EVENT_EXIT"; | ||
| case Py_CONTEXT_SWITCHED: | ||
| return "Py_CONTEXT_SWITCHED"; | ||
| default: | ||
| return "?"; |
There was a problem hiding this comment.
Actually, please you use Py_UNREACHABLE() here
There was a problem hiding this comment.
The ? was @gvanrossum idea in my pr.
There was a problem hiding this comment.
Hm, alright, I'm not gonna overturn Guido's review :) I'll push this as is.
|
Thank you Richard! |
Eclips4
added a commit
to Eclips4/cpython
that referenced
this pull request
Oct 15, 2024
…ython#124776)" This reverts commit 843d28f.
hugovk
pushed a commit
that referenced
this pull request
Oct 15, 2024
Eclips4
added a commit
to Eclips4/cpython
that referenced
this pull request
Oct 15, 2024
…events with "switched" (python#124776)" (python#125513)" This reverts commit d3c82b9.
Eclips4
pushed a commit
to Eclips4/cpython
that referenced
this pull request
Oct 16, 2024
…4776) Users want to know when the current context switches to a different context object. Right now this happens when and only when a context is entered or exited, so the enter and exit events are synonymous with "switched". However, if the changes proposed for pythongh-99633 are implemented, the current context will also switch for reasons other than context enter or exit. Since users actually care about context switches and not enter or exit, replace the enter and exit events with a single switched event. The former exit event was emitted just before exiting the context. The new switched event is emitted after the context is exited to match the semantics users expect of an event with a past-tense name. If users need the ability to clean up before the switch takes effect, another event type can be added in the future. It is not added here because YAGNI. I skipped 0 in the enum as a matter of practice. Skipping 0 makes it easier to troubleshoot when code forgets to set zeroed memory, and it aligns with best practices for other tools (e.g., https://protobuf.dev/programming-guides/dos-donts/#unspecified-enum).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Users want to know when the current context switches to a different context object. Right now this happens when and only when a context is entered or exited, so the enter and exit events are synonymous with "switched". However, if the changes proposed for gh-99633 are implemented, the current context will also switch for reasons other than context enter or exit. Since users actually care about context switches and not enter or exit, replace the enter and exit events with a single switched event.
The former exit event was emitted just before exiting the context. The new switched event is emitted after the context is exited to match the semantics users expect of an event with a past-tense name. If users need the ability to clean up before the switch takes effect, another event type can be added in the future. It is not added here because YAGNI.
This commit amends a feature that is new to v3.14 so I did not include a NEWS blurb (the existing blurb should suffice).
cc @fried
contextvars.Context#99633📚 Documentation preview 📚: https://cpython-previews--124776.org.readthedocs.build/