Skip to content
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

gh-76991: Allow accessing the ags_gen and agt_gen attrs of async generators #11166

Closed

Conversation

ZackerySpytz
Copy link
Contributor

@ZackerySpytz ZackerySpytz commented Dec 14, 2018

Copy link
Contributor

@asvetlov asvetlov left a comment

Choose a reason for hiding this comment

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

Looks very good, thanks!

Would you add a docs update to reflect new fields?

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@1st1
Copy link
Member

1st1 commented Dec 15, 2018

Please don't merge this. I'm not sure i want them to be exposed.

Copy link
Member

@1st1 1st1 left a comment

Choose a reason for hiding this comment

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

We need to consider this carefully. Exposing these fields pins the current implementation and might prevent us from evolving it. It's also unclear if it's possible to implement this in PyPy et al

@asvetlov
Copy link
Contributor

@1st1 thank you for sharing your worries.
I'll assign you to the PR.
You will decide, ok?

@asvetlov asvetlov assigned asvetlov and 1st1 and unassigned asvetlov Dec 15, 2018
@csabella
Copy link
Contributor

@1st1, just wondering if there is any update for this idea? Should the PR be closed so that the discussion can happen on the bug tracker first? Thanks!

@vxgmichel
Copy link
Contributor

We need to consider this carefully. Exposing these fields pins the current implementation and might prevent us from evolving it. It's also unclear if it's possible to implement this in PyPy et al

Another way of fixing the issue described in bpo-32810 without adding extra field names is to expose async_generator.ag_* as async_generator_asend.cr_* (and async_generator_athrow.cr_*), i.e:

  • async_generator_asend.cr_await -> async_generator_asend.ags_gen.ag_await
  • async_generator_asend.cr_code -> async_generator_asend.ags_gen.ag_code
  • async_generator_asend.cr_frame -> async_generator_asend.ags_gen.ag_frame
  • async_generator_asend.cr_running -> async_generator_asend.ags_gen.ag_running
  • async_generator_athrow.cr_await -> async_generator_athrow.agt_gen.ag_await
  • async_generator_athrow.cr_code -> async_generator_athrow.agt_gen.ag_code
  • async_generator_athrow.cr_frame -> async_generator_athrow.agt_gen.ag_frame
  • async_generator_athrow.cr_running -> async_generator_athrow.agt_gen.ag_running

Retrieving the stack can then be implemented by simply walking up the chain of coroutines using cr_await and cr_frame. This would work transparently whether the stack contains async generators or not.

@iritkatriel
Copy link
Member

https://bugs.python.org/issue32810 is closed. What is the status of this PR?

@erlend-aasland erlend-aasland changed the title bpo-32810: Allow accessing the ags_gen and agt_gen attrs of async generators gh-76991: Allow accessing the ags_gen and agt_gen attrs of async generators Jan 5, 2024
@erlend-aasland

This comment was marked as outdated.

@erlend-aasland
Copy link
Contributor

I fixed the merge conflicts, but the tests are failing all over the place. It looks to me the assumptions in the test in 6024d01 from 2018 won't hold in 2023.

@gvanrossum
Copy link
Member

I am just closing this. In 2018 @1st1 didn't like the idea of exposing these fields. Here we are 6 years later and the only reason we're back looking at this is that someone decided to audit the OP's PRs.

@gvanrossum gvanrossum closed this Jan 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet