bpo-42972: Fully implement GC protocol for sqlite3 heap types

[erlend-aasland]

https://bugs.python.org/issue42972

[erlend-aasland]

@pablogsal May I add skip news for this PR?

[erlend-aasland]

@pablogsal:

• I've added tp_clear functions to (almost) all types, since it is recommended by both the doc's and the dev guide. (It also improves readability, IMO.)
• In the tp_clear functions, I've converted Py_(X)DECREF's to Py_CLEAR. It that a problem for objects where we know multiple objects have strong refs. to them?
• Can I normalise the naming for the dealloc functions (=> drop the pysqlite_ prefix)?
• I've added Py_VISIT's for all containers and objects that are passed to us from "Py space"

[pablogsal]

Will review this as soon as possible, but ping me if I haven't done this week :)

[erlend-aasland]

Will review this as soon as possible, but ping me if I haven't done this week :)

No stress; I'm trying to figure out a way to reliably test this :)

[pablogsal]

• In the tp_clear functions, I've converted Py_(X)DECREF's to Py_CLEAR. It that a problem for objects where we know multiple objects have strong refs. to them?

Not sure I understand the question correctly, but in general Py_CLEAR is always better. It is the recommended and safe way to clear data attributes of arbitrary types while decrementing their reference counts. If you were to call Py_XDECREF() instead on the attribute before setting it to NULL, there is a possibility that the attribute’s destructor would call back into code that reads the attribute again (especially if there is a reference cycle).

• Can I normalise the naming for the dealloc functions (=> drop the pysqlite_ prefix)?

Go ahead!

• I've added Py_VISIT's for all containers and objects that are passed to us from "Py space"

👍

[erlend-aasland]

Not sure I understand the question correctly, but in general Py_CLEAR is always better.

No worries, I was mixing up some ref. count concepts. You can disregard that question :)

[erlend-aasland]

@pablogsal Would you mind reviewing the changes? I cannot come up with a way to reliably test this; I guess that would have been easier if sqlite3 had a module state and used multi-phase init (long way to get there).

[pablogsal]

Can you also make a refleak run?

[erlend-aasland]

Can you also make a refleak run?

Yep! That's a normal buildbot run, right, or did you have something else in mind? I've already done a test -R : run locally. @pablogsal

[pablogsal]

Thanks a lot for the patience @erlend-aasland, this is very close to be ready for lending. The GC changes are always tricky so we need to be sure to get them right :)

[erlend-aasland]

Thanks a lot for the patience @erlend-aasland, this is very close to be ready for lending. The GC changes are always tricky so we need to be sure to get them right :)

No problem, I fully agree :)

[bedevere-bot]

🤖 New build scheduled with the buildbot fleet by @pablogsal for commit 926787d 🤖

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

[pablogsal]

LGTM

Great job!

[miss-islington]

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

[bedevere-bot]

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

[erlend-aasland]

Great job!

Fantastic, thank you so much for helping out!