Skip to content

Conversation

@tiran
Copy link
Member

@tirantiran commented Nov 18, 2020

Signed-off-by: Christian Heimes christian@python.org

https://bugs.python.org/issue1635741

Automerge-Triggered-By: GH:tiran

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 668780e30978e11065113c438850074f3fefab5b 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 18, 2020
@pablogsal
Copy link
Member

LGTM, but I added the buildbot label because the gc module has some special handling on deallocation

@tiran
Copy link
MemberAuthor

refleak bots are complaining about a new reference like in test_ast.

@tiran
Copy link
MemberAuthor

refleak bots are complaining about a new reference like in test_ast.

Found it! gcstate->garbage is initialized much earlier at the beginning of an interpreter lifecycle.

@tirantiran added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 19, 2020
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @tiran for commit d1d2cbcb6b14b2f2d8464cb56095b44a4e118ac6 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 19, 2020
Copy link
Member

Choose a reason for hiding this comment

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

This looks wrong to me. If you create multiple instances of the GC modul, callbacks member will be overriden by a new list at each call. It must be initialized exactly once in _PyGC_Init().

By the way, if (gcstate->garbage == NULL){ test in _PyGC_Init() can be removed, it's useless. This function is called exactly once per interpreter.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Makes sense, I'm moving gcstate->callbacks = PyList_New(0) to _PyGC_Init().

Signed-off-by: Christian Heimes <christian@python.org>
Signed-off-by: Christian Heimes <christian@python.org>
@tirantiran added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 19, 2020
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @tiran for commit 7ad98d9 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 19, 2020
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.

LGTM.

@miss-islington
Copy link
Contributor

@tiran: Status check is done, and it's a pending ❌ .

@miss-islington
Copy link
Contributor

@tiran: Status check is done, and it's a success ✅ .

@miss-islingtonmiss-islington merged commit 646d7fd into python:masterNov 19, 2020
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
) Signed-off-by: Christian Heimes <christian@python.org> Automerge-Triggered-By: GH:tiran
Sign up for freeto 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.

6 participants

@tiran@bedevere-bot@pablogsal@miss-islington@vstinner@the-knights-who-say-ni