Skip to content

gh-119333: Add c-api to have contextvar enter/exit callbacks - #119335

Merged
gvanrossum merged 16 commits into
python:mainfrom
fried:pull-request-context-watchers
Sep 24, 2024
Merged

gh-119333: Add c-api to have contextvar enter/exit callbacks#119335
gvanrossum merged 16 commits into
python:mainfrom
fried:pull-request-context-watchers

Conversation

@fried

@friedfried commented May 21, 2024•
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Contributor

This PR implements gh-119333, allowing for setting a callback function on PyContext_Enter and PyContext_Exit from the c-api

Co-authored-by: @itamaro

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

@bedevere-appbedevere-appBot mentioned this pull request May 21, 2024
Closed
1st1
1st1 reviewed May 21, 2024
View reviewed changes
Comment threadPython/context.c
1st1
1st1 reviewed May 21, 2024
View reviewed changes

@1st11st1 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm OK with this change. The motivation for adding it seems reasonable. I'll let @ericsnowcurrently review it properly, but I skimmed through the code and I like the approach.

erlend-aasland
erlend-aasland reviewed May 22, 2024
View reviewed changes

@erlend-aaslanderlend-aasland left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Some minor remarks after a quick look: please align the naming with PEP-7 and use Py (not PY) prefixes for the macros.

Comment threadDoc/c-api/contextvars.rst Outdated
@fried

fried commented May 22, 2024

Copy link
Copy Markdown
ContributorAuthor

@ericsnowcurrently Am I doing this wrong, Having an issue with Check if generated files are up to date I followed the example for the other watcher checks and they use the same style globals. Also I can't run the check locally it on my mac it doesn't seem to like clang.

erlend-aasland
erlend-aasland reviewed May 22, 2024
View reviewed changes
Comment threadDoc/c-api/contextvars.rst Outdated
Comment threadDoc/c-api/contextvars.rst Outdated
Comment threadModules/_testcapi/watchers.c
Comment threadPython/context.c Outdated
Comment threadPython/context.c Outdated
Comment threadPython/context.c Outdated
Comment threadInclude/cpython/context.h Outdated
Comment threadInclude/cpython/context.h Outdated
@ericsnowcurrently

ericsnowcurrently commented May 22, 2024

Copy link
Copy Markdown
Member

Am I doing this wrong, Having an issue with Check if generated files are up to date I followed the example for the other watcher checks and they use the same style globals.

Try copying similar entries in Tools/c-analyzer/cpython/ignored.tsv and then editing them to match the new variables.

Also I can't run the check locally it on my mac it doesn't seem to like clang.

Yeah, sorry, this is definitely a gap.

@fried
friedforce-pushed the pull-request-context-watchers branch from e2fef0c to c210351CompareSeptember 23, 2024 20:01
gvanrossum
gvanrossum reviewed Sep 23, 2024
View reviewed changes
Comment threadPython/context.c
@gvanrossum
gvanrossum merged commit d87482b into python:mainSep 24, 2024
kumaraditya303
kumaraditya303 reviewed Sep 24, 2024
View reviewed changes
Comment threadPython/context.c
static void notify_context_watchers(PyContextEvent event, PyContext *ctx)
{
assert(Py_REFCNT(ctx) > 0);
PyInterpreterState *interp = _PyInterpreterState_GET();

@kumaraditya303kumaraditya303Sep 24, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

FWIW you can get interp from tstate which is already known, it would have avoided one more tls read which is slow on msvc.

@gvanrossumgvanrossumSep 24, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sorry. We’ll fix it tomorrow. Any other concerns?

kumaraditya303
kumaraditya303 reviewed Sep 24, 2024
View reviewed changes
Comment threadInclude/cpython/context.h
} PyContextEvent;

/*
* A Callback to clue in non-python contexts impls about a

@kumaraditya303kumaraditya303Sep 24, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't understand this comment, what is meant by clue in? Also what is change in active python context?

kumaraditya303
kumaraditya303 reviewed Sep 24, 2024
View reviewed changes
Comment threadInclude/cpython/context.h
* A Callback to clue in non-python contexts impls about a
* change in the active python context.
*
* The callback is invoked with the event and a reference to =

@kumaraditya303kumaraditya303Sep 24, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Remove the =, it adds no useful info and is confusing.

@fried
fried deleted the pull-request-context-watchers branch September 24, 2024 16:21
@friedfried mentioned this pull request Sep 24, 2024
Merged
@rhansenrhansen mentioned this pull request Sep 30, 2024
Merged
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@encukouencukouencukou left review comments
@gvanrossumgvanrossumgvanrossum approved these changes
@erlend-aaslanderlend-aaslanderlend-aasland left review comments
@kumaraditya303kumaraditya303kumaraditya303 left review comments
@1st11st11st1 approved these changes
@ericsnowcurrentlyericsnowcurrentlyAwaiting requested review from ericsnowcurrentlyericsnowcurrently is a code owner

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@fried@ericsnowcurrently@1st1@encukou@gvanrossum@erlend-aasland@kumaraditya303