Skip to content

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

Merged
pablogsal merged 14 commits into
python:mainfrom
erlend-aasland:bpo-42972/sqlite
May 25, 2021
Merged

bpo-42972: Fully implement GC protocol for sqlite3 heap types#26104
pablogsal merged 14 commits into
python:mainfrom
erlend-aasland:bpo-42972/sqlite

Conversation

@erlend-aasland

@erlend-aaslanderlend-aasland commented May 13, 2021•
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

@erlend-aasland

erlend-aasland commented May 13, 2021

Copy link
Copy Markdown
ContributorAuthor

@pablogsal May I add skip news for this PR?

@erlend-aasland

erlend-aasland commented May 13, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

@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"

@erlend-aaslanderlend-aasland changed the title bpo-42972: implement full GC support for sqlite3 heap typesbpo-42972: Fully implement GC protocol for sqlite3 heap typesMay 14, 2021
@erlend-aaslanderlend-aasland added the needs backport to 3.10 only security fixes label May 14, 2021
@pablogsal

pablogsal commented May 17, 2021

Copy link
Copy Markdown
Member

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

@erlend-aasland

erlend-aasland commented May 17, 2021

Copy link
Copy Markdown
ContributorAuthor

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

pablogsal commented May 19, 2021

Copy link
Copy Markdown
Member
  • 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"

👍

shihai1991
shihai1991 reviewed May 23, 2021
View reviewed changes
Comment threadModules/_sqlite/cache.c
Comment threadModules/_sqlite/connection.c
@erlend-aasland

erlend-aasland commented May 23, 2021

Copy link
Copy Markdown
ContributorAuthor

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

erlend-aasland commented May 24, 2021

Copy link
Copy Markdown
ContributorAuthor

@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).

@erlend-aaslanderlend-aasland mentioned this pull request May 24, 2021
Merged
pablogsal
pablogsal reviewed May 24, 2021
View reviewed changes
Comment threadModules/_sqlite/prepare_protocol.c
pablogsal
pablogsal reviewed May 24, 2021
View reviewed changes
Comment threadModules/_sqlite/row.c
pablogsal
pablogsal reviewed May 24, 2021
View reviewed changes
Comment threadModules/_sqlite/statement.c Outdated
@pablogsal

pablogsal commented May 24, 2021

Copy link
Copy Markdown
Member

Can you also make a refleak run?

@erlend-aasland

erlend-aasland commented May 25, 2021•
edited
Loading

Copy link
Copy Markdown
ContributorAuthor

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

pablogsal commented May 25, 2021

Copy link
Copy Markdown
Member

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

erlend-aasland commented May 25, 2021

Copy link
Copy Markdown
ContributorAuthor

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 :)

@pablogsalpablogsal added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 25, 2021
@bedevere-bot

bedevere-bot commented May 25, 2021

Copy link
Copy Markdown

🤖 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.

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label May 25, 2021
pablogsal
pablogsal approved these changes May 25, 2021
View reviewed changes

@pablogsalpablogsal left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

Great job!

@pablogsal
pablogsal merged commit d3c277a into python:mainMay 25, 2021
@miss-islington

miss-islington commented May 25, 2021

Copy link
Copy Markdown
Contributor

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request May 25, 2021
Erlend Egeberg Aasland@miss-islington
bpo-42972: Fully implement GC protocol for sqlite3 heap types (python…
da7a8d0
…GH-26104)
(cherry picked from commit d3c277a)
Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
@bedevere-bot

bedevere-bot commented May 25, 2021

Copy link
Copy Markdown

GH-26361 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 25, 2021
@erlend-aasland

erlend-aasland commented May 25, 2021

Copy link
Copy Markdown
ContributorAuthor

Great job!

Fantastic, thank you so much for helping out!

@erlend-aasland
erlend-aasland deleted the bpo-42972/sqlite branch May 25, 2021 17:46
miss-islington added a commit that referenced this pull request May 25, 2021
@miss-islingtonErlend Egeberg Aasland
bpo-42972: Fully implement GC protocol for sqlite3 heap types (GH-26104)
e8d9df0
(cherry picked from commit d3c277a)
Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
@erlend-aaslanderlend-aasland mentioned this pull request May 29, 2021
Merged
@pablogsalpablogsal mentioned this pull request Jul 18, 2021
Closed
@erlend-aaslanderlend-aasland mentioned this pull request Aug 20, 2021
Closed
@vstinnervstinner mentioned this pull request Jun 9, 2022
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shihai1991shihai1991shihai1991 left review comments
@pablogsalpablogsalpablogsal approved these changes
@berkerpeksagberkerpeksagAwaiting requested review from berkerpeksag

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

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