GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-41861: Convert _sqlite3 CursorType and ConnectionType to heap types by erlend-aasland · Pull Request #22478 · python/cpython

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented Oct 1, 2020 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Part 4 of bpo-41861, @vstinner. #norefleaks

After this, I'd like to merge #22419 to finalise bpo-41861.

https://bugs.python.org/issue41861

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
Erlend E. Aasland added 2 commits October 1, 2020 15:27
Erlend E. Aasland
Convert CursorType to head type
5be6718
Erlend E. Aasland
Convert ConnectionType to heap type
77636df
@erlend-aasland
erlend-aasland requested a review from berkerpeksag as a code owner October 1, 2020 13:34
@the-knights-who-say-ni the-knights-who-say-ni added the CLA signed label Oct 1, 2020
@bedevere-bot bedevere-bot added the awaiting review label Oct 1, 2020
@vstinner vstinner added the skip news label Oct 1, 2020
@vstinner
vstinner merged commit 256e54a into python:master Oct 1, 2020
@bedevere-bot bedevere-bot removed the awaiting review label Oct 1, 2020
@vstinner

vstinner commented Oct 1, 2020

Copy link
Copy Markdown
Member

Merged, thanks. There are two remaining static types?

erlend-aasland reacted with hooray emoji
All reactions
  • 🎉 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland
erlend-aasland deleted the fix-bpo-41861/cur-and-con branch October 1, 2020 14:39
@erlend-aasland

erlend-aasland commented Oct 1, 2020

Copy link
Copy Markdown
Contributor Author

Merged, thanks. There are two remaining static types?

Thanks! No more remaining types.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Oct 1, 2020

Copy link
Copy Markdown
Contributor Author

Merged, thanks. There are two remaining static types?

BTW, should I add traverse/clear/free methods to the module def before closing the issue?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Oct 1, 2020 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

BTW, should I add traverse/clear/free methods to the module def before closing the issue?

Currently, it's possible to have more than once instance of the _sqlite3 module. If a second instance is created and then cleared, it would clear the shared heap types of the first instance: not good.

It would be better to add a module state and retrieve these types from the module state.

The problem is to retrieve the module state. I suggest you to convert all methods which use these 4 types to Argument Clinic, so later you will be able to use "defining_class" parameter which is a reliable way to get the module state from a type. That's why I asked you to use PyType_FromModuleAndSpec().

Example: _sqlite3.Connection.backup() Python method is implemented as the pysqlite_connection_backup() C function. If you modify the function to use Argument Clinic, you can then get the defining class: then PyType_GetModuleState(cls) gives you the module state.

pysqlite_connection_backup() -> defining_class (cls) -> PyType_GetModuleState(cls) -> module state -> your heap types

Converting _sqlite3 to use a module state will make it safe to be used in Python subinterpreters!

See:

  • My notes: https://pythondev.readthedocs.io/subinterpreters.html
  • Convert static types to heap types: use PyType_FromSpec(): https://bugs.python.org/issue40077
  • bpo-1635741: Py_Finalize() doesn’t clear all Python objects at exit: https://bugs.python.org/issue1635741
erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Oct 1, 2020

Copy link
Copy Markdown
Contributor Author

BTW, should I add traverse/clear/free methods to the module def before closing the issue?

Currently, it's possible to have more than once instance of the _sqlite3 module. If a second instance is created and then cleared, it would clear the shared heap types of the first instance: not good.

It would be better to add a module state and retrieve these types from the module state.

I already started experimenting with this. Should I put up what I've got as a draft PR? I guess this goes as a separate issue anyways.

The problem is to retrieve the module state.

I'm painfully aware of this :)

I suggest you to convert all methods which use these 4 types to Argument Clinic, so later you will be able to use "defining_class" parameter which is a reliable way to get the module state from a type. That's why I asked you to use PyType_FromModuleAndSpec().

I think I already put up a PR that converts _sqlite3 to Argument Clinic. I'll rebase onto master and ping you.

  • My notes: https://pythondev.readthedocs.io/subinterpreters.html
  • Convert static types to heap types: use PyType_FromSpec(): https://bugs.python.org/issue40077
  • bpo-1635741: Py_Finalize() doesn’t clear all Python objects at exit: https://bugs.python.org/issue1635741

Thanks!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Oct 1, 2020

Copy link
Copy Markdown
Member

a PR that converts _sqlite3 to Argument Clinic

That sounds like a good start! I prefer a PR to convert to Argument Clinic, and then a second one to add a module state.

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Oct 13, 2020 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

a PR that converts _sqlite3 to Argument Clinic

That sounds like a good start! I prefer a PR to convert to Argument Clinic, and then a second one to add a module state.

While my other PR's are on hold, I've started with the post-agrument-clinic-step: heap module state (PEP 573). I'm currently basing this branch on top off my work with bpo-40956. Using defining_class makes this very easy, however I need help with resolving two issues:

  1. What to do with the Argument Clinic class definition? We need the module pointer to get to the instances and the types. Quoting from the Argument Clinic docs:
    "When you declare a class, you must also specify two aspects of its type in C: the type declaration you’d use for a pointer to an instance of this class, and a pointer to the PyTypeObject for this class."

  2. What to do with __init__ and __new__? Providing a pointer to the module in the class/type state, would solve it, but that feels backwards and hacky.

UPDATE: Solved like this (excerpt from Argument Clinic for _sqlite3.Connection.__init__):
factory: object(c_default='(PyObject*)((struct pysqlite_state *)PyType_GetModuleState(Py_TYPE(self)))->ConnectionType') = ConnectionType
Is this an accepted solution?

Also, defining_class is not documented. I'll open an issue for that. https://docs.python.org/3.10/howto/clinic.html

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Oct 15, 2020

Copy link
Copy Markdown
Member

Also, defining_class is not documented. I'll open an issue for that. https://docs.python.org/3.10/howto/clinic.html

Good idea.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@vstinner

vstinner commented Oct 15, 2020

Copy link
Copy Markdown
Member

I wrote #22712 to "explain my idea". It's a minimum change just to pass a "state" to functions which access the UCD_Type.

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@erlend-aasland

erlend-aasland commented Oct 15, 2020

Copy link
Copy Markdown
Contributor Author

I wrote #22712 to "explain my idea". It's a minimum change just to pass a "state" to functions which access the UCD_Type.

Thanks, I'll have a look.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

xzy3 pushed a commit to xzy3/cpython that referenced this pull request Oct 18, 2020
Erlend Egeberg Aasland @xzy3-cdc
bpo-41861: Convert _sqlite3 CursorType and ConnectionType to heap typ…
8d8d60d
…es (pythonGH-22478)
@erlend-aasland erlend-aasland mentioned this pull request Dec 28, 2020
bpo-42048: Document AC's defining_class converter #23978
Merged
@erlend-aasland erlend-aasland mentioned this pull request Aug 20, 2021
bpo-24905: Support BLOB incremental I/O in sqlite module #271
Closed
@kylotan kylotan mannequin mentioned this pull request Sep 19, 2022
Py_Finalize() doesn't clear all Python objects at exit #44470
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@erlend-aasland @vstinner @the-knights-who-say-ni @bedevere-bot
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.