Skip to content

gh-129889: Support context manager protocol by contextvars.Token #129888

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 13 commits into from
Feb 12, 2025

Conversation

asvetlov
Copy link
Contributor

@asvetlov asvetlov commented Feb 9, 2025

@asvetlov asvetlov changed the title Support context manager protocol by contextvars.Token gh-129889: Support context manager protocol by contextvars.Token Feb 9, 2025
Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Since it's a new feature, we also need a What's New entry as well (I think you already know it and since it's a draft, you might be writing it at the moment, but it's just a friendly reminder!).

@@ -383,6 +383,31 @@ def sub(num):
tp.shutdown()
self.assertEqual(results, list(range(10)))

def test_token_contextmanager_with_default(self):
Copy link
Member

Choose a reason for hiding this comment

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

Maybe more test with a re-entrant context? as well as a test when an exception is raised in the context body?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review, I'll add tests.
I don't expect any problems though since the implementation is really trivial.

Copy link
Member

@picnixz picnixz Feb 9, 2025

Choose a reason for hiding this comment

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

Up to you though! Generally, I'm not actually adding those kind of tests to test the implementation but rather to spot possible regressions if we change something. I actually don't know if we are that pedantic when testing other context managers so feel free not to burden the tests if you think it's not worth it!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests are cheap and easy to write,
I've added two: one for exit when an exception is raised and another for reentrancy.

Please feel free to ask for additional tests if needed.

Copy link
Member

Choose a reason for hiding this comment

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

Looks good! Small question, but is there a necessity to check what happens if we call c.reset() inside the context manager? or multiple c.set() as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Multiple c.set() doesn't affect the context manager, the value will be reset on exit.
c.reset() is safe if the other token was used; resetting with the same token raises RuntimeErorr as in the following example:

with c.set(1) as token:
    c.reset(token)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A couple additional tests were added to demonstrate mentioned scenarios.

Copy link
Member

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Thanks for those tests! I think they are worth it since they capture non-trivial cases even if the implementation is trivial. Except for the changelog entries to be written I think we got a pretty good coverage!

@asvetlov asvetlov marked this pull request as ready for review February 10, 2025 08:50
@asvetlov
Copy link
Contributor Author

@picnixz news is added, and the PR is ready for review.

@1st1, maybe you can find time for the review as the author of contextvars?

Copy link
Member

@picnixz picnixz 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!

@asvetlov asvetlov merged commit 469d2e4 into python:main Feb 12, 2025
42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support context manager protocol by contextvars.Token
2 participants