Skip to content

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

Merged
bors[bot] merged 4 commits intomainfrom
no-gil-in-traverse
May 25, 2023
Merged

Do not apply deferred ref count updates and prevent the GIL from being acquired inside of __traverse__ implementations.#3168
bors[bot] merged 4 commits intomainfrom
no-gil-in-traverse

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 commented May 20, 2023
View reviewed changes
@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...

@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

adamreichold commented May 25, 2023

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 <adam.reichold@t-online.de>
@bors
Copy link
Contributor

bors bot commented May 25, 2023

Build failed:

@adamreichold
Copy link
Member Author

adamreichold commented May 25, 2023

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 <public+git@mearie.org>
@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

Development

Successfully merging this pull request may close these issues.

__traverse__ should not alter any refcounts, at least in CPython Raising warning inside __traverse__ causes a segfault

2 participants

@adamreichold @davidhewitt