Skip to content

[sqlite3] cleanup callbacks (GIL handling, naming, ...) #89154

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
erlend-aasland opened this issue Aug 24, 2021 · 9 comments
Closed

[sqlite3] cleanup callbacks (GIL handling, naming, ...) #89154

erlend-aasland opened this issue Aug 24, 2021 · 9 comments
Assignees
Labels
extension-modules C modules in the Modules dir topic-sqlite3 type-feature A feature request or enhancement

Comments

@erlend-aasland
Copy link
Contributor

BPO 44991
Nosy @encukou, @corona10, @miss-islington, @erlend-aasland
PRs
  • bpo-44991: Make GIL handling more explicit in sqlite3 callbacks #27934
  • bpo-44991: Normalise sqlite3 callback naming #28088
  • bpo-44991: Normalise sqlite3 function and collation callback naming #28209
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = 'https://github.com/erlend-aasland'
    closed_at = <Date 2021-09-07.13:06:38.494>
    created_at = <Date 2021-08-24.13:07:12.425>
    labels = ['extension-modules', 'type-feature']
    title = '[sqlite3] cleanup callbacks (GIL handling, naming, ...)'
    updated_at = <Date 2021-10-12.11:39:20.178>
    user = 'https://github.com/erlend-aasland'

    bugs.python.org fields:

    activity = <Date 2021-10-12.11:39:20.178>
    actor = 'petr.viktorin'
    assignee = 'erlendaasland'
    closed = True
    closed_date = <Date 2021-09-07.13:06:38.494>
    closer = 'petr.viktorin'
    components = ['Extension Modules']
    creation = <Date 2021-08-24.13:07:12.425>
    creator = 'erlendaasland'
    dependencies = []
    files = []
    hgrepos = []
    issue_num = 44991
    keywords = ['patch']
    message_count = 9.0
    messages = ['400207', '400225', '400265', '400267', '400269', '400722', '401246', '401247', '403726']
    nosy_count = 4.0
    nosy_names = ['petr.viktorin', 'corona10', 'miss-islington', 'erlendaasland']
    pr_nums = ['27934', '28088', '28209']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue44991'
    versions = []

    @erlend-aasland
    Copy link
    Contributor Author

    Quoting msg400205 by Petr in bpo-42064:
    I think the module could use a more comprehensive review for GIL handling, rather than doing it piecewise in individual PRs. I recommend that any function passed to SQLite (and only those) should

    • be named *_callback, for clarity
    • acquire the GIL at the very start
    • release the GIL at the very end

    @erlend-aasland erlend-aasland self-assigned this Aug 24, 2021
    @erlend-aasland erlend-aasland added extension-modules C modules in the Modules dir type-feature A feature request or enhancement labels Aug 24, 2021
    @erlend-aasland erlend-aasland self-assigned this Aug 24, 2021
    @erlend-aasland erlend-aasland added extension-modules C modules in the Modules dir type-feature A feature request or enhancement labels Aug 24, 2021
    @erlend-aasland
    Copy link
    Contributor Author

    I'd like to propose further enhancements:

    • use intermingled declarations in the affected functions; this will make GIL acquisition stand more out, and it also improves readability and makes it easier to trace refs
    • take the naming step further: I'd like to normalise PyObject * callback variable names, the extremely long function_pinboard_* names, and also drop the C callback prefixes ('_' and 'pysqlite_')

    If you agree, I'll create separate PR's for those; one for each refactor. I think it will enhance readability a lot.

    @erlend-aasland erlend-aasland changed the title [sqlite3] cleanup GIL handling [sqlite3] cleanup callbacks (GIL handling, naming, ...) Aug 24, 2021
    @erlend-aasland erlend-aasland changed the title [sqlite3] cleanup GIL handling [sqlite3] cleanup callbacks (GIL handling, naming, ...) Aug 24, 2021
    @encukou
    Copy link
    Member

    encukou commented Aug 25, 2021

    The general policy is to only improve style for code you touch for some other reasons. You're changing lots of the callback code in bpo-42064, so I reckon making things more readable is fine.

    @corona10
    Copy link
    Member

    Obviously, this case could be code churn(we don't accept normally), but if other core-dev agree with the changes. I am fine.

    @erlend-aasland
    Copy link
    Contributor Author

    We'll do the changes Petr proposed first, and then I'll see how invasive the other changes will be. If the diffs end up being concise, I'll put them up for review :)

    @miss-islington
    Copy link
    Contributor

    New changeset 001ef46 by Erlend Egeberg Aasland in branch 'main':
    bpo-44991: Make GIL handling more explicit in sqlite3 callbacks (GH-27934)
    001ef46

    @encukou
    Copy link
    Member

    encukou commented Sep 7, 2021

    New changeset 0474d06 by Erlend Egeberg Aasland in branch 'main':
    bpo-44991: Normalise sqlite3 callback naming (GH-28088)
    0474d06

    @encukou
    Copy link
    Member

    encukou commented Sep 7, 2021

    The remaining function_pinboard_* renames are part of PR-27940.

    @encukou encukou closed this as completed Sep 7, 2021
    @encukou encukou closed this as completed Sep 7, 2021
    @encukou
    Copy link
    Member

    encukou commented Oct 12, 2021

    New changeset cfb1df3 by Erlend Egeberg Aasland in branch 'main':
    bpo-44991: Normalise function and collation callback naming (GH-28209)
    cfb1df3

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    extension-modules C modules in the Modules dir topic-sqlite3 type-feature A feature request or enhancement
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants