Skip to content

Conversation

@erlend-aasland
Copy link
Contributor

@erlend-aaslanderlend-aasland commented May 25, 2021

break;
}
}
PyObject_GC_Track(self);
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

@pablogsal I'm unsure if this it the correct place to start tracking the newly created pattern object. My reasoning was that AFAICS, it seems to be fully initialised here, unless I've missed something.

Copy link
Member

Choose a reason for hiding this comment

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

IMO it's the right place.

It must be called before the first DECREF(self), otherwise PyObject_GC_UnTrack() crash in dealloc.

Usually, it's better to only track an object once it's fully initialized, but here it's more convient to do it here to be able to simply call DECREF() on error. The bare minimum is that traverse must be crash when an object started to be tracked. I added a test in debug mode for that: see traverse(visit_validate) in PyObject_GC_Track().

@erlend-aaslanderlend-aasland added the needs backport to 3.10 only security fixes label May 25, 2021
Copy link
Member

@vstinnervstinner left a comment

Choose a reason for hiding this comment

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

I'm not sure that calling PyObject_ClearWeakRefs() in a clear function is safe. What do you think?

break;
}
}
PyObject_GC_Track(self);
Copy link
Member

Choose a reason for hiding this comment

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

IMO it's the right place.

It must be called before the first DECREF(self), otherwise PyObject_GC_UnTrack() crash in dealloc.

Usually, it's better to only track an object once it's fully initialized, but here it's more convient to do it here to be able to simply call DECREF() on error. The bare minimum is that traverse must be crash when an object started to be tracked. I added a test in debug mode for that: see traverse(visit_validate) in PyObject_GC_Track().

@vstinnervstinner merged commit fba42d1 into python:mainMay 27, 2021
@miss-islington
Copy link
Contributor

Thanks @erlend-aasland for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-26411 is a backport of this pull request to the 3.10 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 27, 2021
(cherry picked from commit fba42d1) Co-authored-by: Erlend Egeberg Aasland <[email protected]>
@erlend-aaslanderlend-aasland deleted the bpo-42972/re branch May 27, 2021 17:41
@vstinnervstinner added the needs backport to 3.10 only security fixes label May 27, 2021
@miss-islington
Copy link
Contributor

Thanks @erlend-aasland for the PR, and @vstinner for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 27, 2021
(cherry picked from commit fba42d1) Co-authored-by: Erlend Egeberg Aasland <[email protected]>
@bedevere-bot
Copy link

GH-26414 is a backport of this pull request to the 3.10 branch.

@bedevere-botbedevere-bot removed the needs backport to 3.10 only security fixes label May 27, 2021
vstinner pushed a commit that referenced this pull request May 28, 2021
) (cherry picked from commit fba42d1) Co-authored-by: Erlend Egeberg Aasland <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@erlend-aasland@miss-islington@bedevere-bot@vstinner@the-knights-who-say-ni