GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-124872: Replace enter/exit events with "switched" by rhansen · Pull Request #124776 · python/cpython

Conversation

@rhansen

@rhansen rhansen commented Sep 30, 2024 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

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

  • Issue: Add c-api to set callback function on enter and exit of PyContext #119333
  • Issue: Context manager support for contextvars.Context #99633
  • Issue: Replace context enter and exit events with single "context switched" event #124872

📚 Documentation preview 📚: https://cpython-previews--124776.org.readthedocs.build/

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@bedevere-app bedevere-app Bot mentioned this pull request Sep 30, 2024
Add c-api to set callback function on enter and exit of PyContext #119333
Closed
This was referenced Sep 30, 2024
Context manager support for contextvars.Context #99633
Open
gh-124872: Back up exception before calling PyContext_WatchCallback #124741
Open
@rhansen
rhansen force-pushed the gh-119333-context-switched branch 4 times, most recently from 8242dce to 7606b67 Compare October 1, 2024 23:23
@rhansen rhansen changed the title gh-119333, gh-99633: Replace enter/exit events with "switched" gh-124872: Replace enter/exit events with "switched" Oct 1, 2024
@bedevere-app bedevere-app Bot mentioned this pull request Oct 1, 2024
Replace context enter and exit events with single "context switched" event #124872
Open
@rhansen
rhansen force-pushed the gh-119333-context-switched branch 3 times, most recently from e68cb7f to d04d9c0 Compare October 2, 2024 22:04
@rhansen
rhansen force-pushed the gh-119333-context-switched branch from d04d9c0 to f3f17b3 Compare October 10, 2024 08:45
picnixz
picnixz reviewed Oct 12, 2024
View reviewed changes
Comment thread Doc/c-api/contextvars.rst Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_testcapi/watchers.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_testcapi/watchers.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread Modules/_testcapi/watchers.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

@ZeroIntensity ZeroIntensity added the skip news label Oct 12, 2024
@rhansen
rhansen force-pushed the gh-119333-context-switched branch from f3f17b3 to b4cff77 Compare October 12, 2024 21:37
@rhansen
pythongh-124872: Replace enter/exit events with "switched"
d898525
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
rhansen force-pushed the gh-119333-context-switched branch from b4cff77 to d898525 Compare October 12, 2024 21:40
@rhansen
rhansen marked this pull request as ready for review October 12, 2024 21:42
@rhansen
rhansen requested review from 1st1 and ericsnowcurrently as code owners October 12, 2024 21:42
@bedevere-app bedevere-app Bot added the awaiting review label Oct 12, 2024
ZeroIntensity
ZeroIntensity approved these changes Oct 12, 2024
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting core review and removed awaiting review labels Oct 12, 2024
@ZeroIntensity

ZeroIntensity commented Oct 12, 2024

Copy link
Copy Markdown
Member

This is sort of reviewed already, per discussion in the original issue, so I'm approving it.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

fried
fried approved these changes Oct 13, 2024
View reviewed changes

@fried fried left a comment

Copy link
Copy Markdown
Contributor

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.

LGTM on behavioral changes. @1st1 as requested

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

1st1 reacted with rocket emoji
All reactions
  • 🚀 1 reaction
1st1
1st1 approved these changes Oct 14, 2024
View reviewed changes
@bedevere-app bedevere-app Bot added awaiting merge and removed awaiting core review labels Oct 14, 2024
1st1
1st1 reviewed Oct 14, 2024
View reviewed changes
Comment thread Python/context.c
case Py_CONTEXT_SWITCHED:
return "Py_CONTEXT_SWITCHED";
default:
return "?";

@1st1 1st1 Oct 14, 2024

Copy link
Copy Markdown
Member

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.

Actually, please you use Py_UNREACHABLE() here

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@fried fried Oct 14, 2024

Copy link
Copy Markdown
Contributor

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.

The ? was @gvanrossum idea in my pr.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

@1st1 1st1 Oct 14, 2024

Copy link
Copy Markdown
Member

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.

Hm, alright, I'm not gonna overturn Guido's review :) I'll push this as is.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@1st1
1st1 merged commit 843d28f into python:main Oct 14, 2024
@bedevere-app bedevere-app Bot removed the awaiting merge label Oct 14, 2024
@1st1

1st1 commented Oct 14, 2024

Copy link
Copy Markdown
Member

Thank you Richard!

rhansen reacted with heart emoji
All reactions
  • ❤️ 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@rhansen
rhansen deleted the gh-119333-context-switched branch October 15, 2024 00:03
Eclips4 added a commit to Eclips4/cpython that referenced this pull request Oct 15, 2024
@Eclips4
Revert "pythongh-124872: Replace enter/exit events with "switched" (p…
17ee341
…ython#124776)"

This reverts commit 843d28f.
hugovk pushed a commit that referenced this pull request Oct 15, 2024
@Eclips4
gh-125512: Revert "gh-124872: Replace enter/exit events with "switche…
d3c82b9
…d" (#124776)" (#125513)
Eclips4 added a commit to Eclips4/cpython that referenced this pull request Oct 15, 2024
@Eclips4
Revert "pythongh-125512: Revert "pythongh-124872: Replace enter/exit …
f66fbef
…events with "switched" (python#124776)" (python#125513)"

This reverts commit d3c82b9.
@ZeroIntensity ZeroIntensity mentioned this pull request Oct 15, 2024
Adding an internal C-API for dynamic arrays #125543
Closed
Eclips4 pushed a commit to Eclips4/cpython that referenced this pull request Oct 16, 2024
@rhansen @Eclips4
pythongh-124872: Replace enter/exit events with "switched" (python#12…
f0b3aa8
…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

Reviewers

@fried fried fried approved these changes
@1st1 1st1 1st1 approved these changes
@picnixz picnixz picnixz left review comments
@ZeroIntensity ZeroIntensity ZeroIntensity approved these changes
@ericsnowcurrently ericsnowcurrently Awaiting requested review from ericsnowcurrently ericsnowcurrently is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@rhansen @ZeroIntensity @1st1 @fried @picnixz
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.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.