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.

Sign up for GitHub

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

Jump to bottom

Do not apply deferred ref count updates and prevent the GIL from being acquired inside of __traverse__ implementations. #3168

Merged
merged 4 commits into from
May 25, 2023
Merged

Do not apply deferred ref count updates and prevent the GIL from being acquired inside of __traverse__ implementations. #3168

merged 4 commits into from
May 25, 2023

Conversation

adamreichold
Copy link
Member

@adamreichold adamreichold commented May 20, 2023 •
edited
Loading

Closes #2301
Closes #3165

@adamreichold adamreichold mentioned this pull request May 20, 2023
Closed
@adamreichold adamreichold force-pushed the no-gil-in-traverse branch 6 times, most recently from 3ee383e to d6fb513 Compare May 20, 2023 13:34
@adamreichold adamreichold marked this pull request as ready for review May 20, 2023 13:34
This was referenced May 21, 2023
Closed
Closed
davidhewitt
davidhewitt reviewed May 23, 2023
View reviewed changes
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Nice, yes I'm happy with things going in this direction. I added #2301 to the "closes" section in your OP.

I had quite a few tangential thoughts as I read through this one...

src/gil.rs Show resolved Hide resolved
src/gil.rs Outdated Show resolved Hide resolved
src/impl_/pymethods.rs Outdated Show resolved Hide resolved
pyo3-macros-backend/src/pymethod.rs Show resolved Hide resolved
tests/ui/traverse_bare_self.stderr Show resolved Hide resolved
@adamreichold adamreichold force-pushed the no-gil-in-traverse branch 2 times, most recently from 0ab1365 to 89a6c43 Compare May 24, 2023 08:17
davidhewitt
davidhewitt approved these changes May 25, 2023
View reviewed changes
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Looks great to me, thanks!

I see you have linked this to #3064, it's not quite clear to me why?

@adamreichold
Copy link
Member Author

I see you have linked this to #3064, it's not quite clear to me why?

I misunderstood it as one of the symptoms described in #3165 but reading it less tired, that was nonsense. I removed the reference will give bors a try.

bors r=davidhewitt

bors bot added a commit that referenced this pull request May 25, 2023
@bors @adamreichold
Merge #3168
efaf768 
3168: Do not apply deferred ref count updates and prevent the GIL from being acquired inside of __traverse__ implementations. r=davidhewitt a=adamreichold

Closes #2301
Closes #3165


Co-authored-by: Adam Reichold <[email protected]>
@bors
Copy link
Contributor

bors bot commented May 25, 2023

Build failed:

@adamreichold
Copy link
Member Author

So this was hit by the same problem that #3171 is trying to fix. I just moved the test case to the 1.63 section for now.

bors retry

@bors
Copy link
Contributor

bors bot commented May 25, 2023

Build succeeded!

The publicly hosted instance of bors-ng is deprecated and will go away soon.

If you want to self-host your own instance, instructions are here.
For more help, visit the forum.

If you want to switch to GitHub's built-in merge queue, visit their help page.

@bors bors bot merged commit 32c335e into main May 25, 2023
@bors bors bot deleted the no-gil-in-traverse branch May 25, 2023 19:21
bors bot added a commit that referenced this pull request May 25, 2023
@bors @lifthrasiir
Merge #3184
2ed1d70 
3184: Additional tests for #3168 r=adamreichold a=adamreichold

These were a part of tests `@lifthrasiir` was preparing for #3165, and I believe it's worthy to add them (any single of them fails in the current main branch).

Co-authored-by: Kang Seonghoon <[email protected]>
@mejrs mejrs mentioned this pull request Jun 19, 2023
Merged
@neachdainn neachdainn mentioned this pull request Nov 30, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@davidhewitt davidhewitt davidhewitt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
2 participants
@adamreichold @davidhewitt