8000 Docs: improve accuracy of sqlite3 `check_same_thread` parameter by marcospgp · Pull Request #101351 · python/cpython · GitHub
[go: up one dir, main page]
Skip to content
8000

Docs: improve accuracy of sqlite3 check_same_thread parameter #101351

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Feb 1, 2023
Merged

Docs: improve accuracy of sqlite3 check_same_thread parameter #101351

merged 6 commits into from
Feb 1, 2023

Conversation

marcospgp
Copy link
Contributor

Previous information was false: setting check_same_thread to False does not always require that the user ensure sequentialization themselves, depending on the value of threadsafety.

@ghost
Copy link
8000 ghost commented Jan 26, 2023 •
edited by ghost
Loading

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-bot bedevere-bot added awaiting review docs Documentation in the Doc dir skip news labels Jan 26, 2023
@erlend-aasland erlend-aasland self-assigned this Jan 26, 2023
@erlend-aasland erlend-aasland changed the title Update check_same_thread description in sqlite3.rst Docs: improve accuracy of sqlite3 check_same_thread parameter Jan 26, 2023
erlend-aasland
erlend-aasland reviewed Jan 26, 2023
View reviewed changes
@marcospgp
Copy link
Contributor Author

Looks good to me, committed!

@marcospgp
Copy link
Contributor Author

Any chance to change "serialized" into "sequentialized" here to contrast with serialization as storing state into bytes?

@erlend-aasland
Copy link
Contributor

Any chance to change "serialized" into "sequentialized" here to contrast with serialization as storing state into bytes?

Interesting observation. Perhaps the wording can be improved there, however I find "sequentialized" a little bit off-kilter (but perhaps that's just me). @CAM-Gerlach?

CAM-Gerlach
CAM-Gerlach reviewed Jan 28, 2023
View reviewed changes
@marcospgp
Copy link
Contributor Author

@CAM-Gerlach I took your suggestions and believe I have arrived at a simplified solution. Let me know what you think as well as @erlend-aasland . Feel free to modify at will!

erlend-aasland
erlend-aasland reviewed Jan 29, 2023
View reviewed changes
Copy link
Contributor
@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

Thanks; the first sentence is a welcome improvement! I believe the second sentence need a little bit more care before we can land this :)

CAM-Gerlach
CAM-Gerlach reviewed Jan 30, 2023
View reviewed changes
@marcospgp
Copy link
Contributor Author
marcospgp commented Jan 30, 2023 •
edited
Loading

I committed your suggestion @CAM-Gerlach , looks great to me!

I think this might require a little more work in order to make clear the initial message I was intending to add: that if SQLite is in serialized mode, check_same_thread can be safely set to False (supposedly - have not run into data corruption so far, but admittedly very hard to verify).

Pointing to :attr:`threadsafety` is already a step in that direction, but perhaps we could be a little more obvious?

To be clear in motivation - this issue stems from using Flask with SQLite. Their official guide suggests opening and closing an sqlite3 database connection for each incoming request, which is unnecessarily inefficient if a single (or a pool of) global connection(s) can be safely shared.

What makes the global shared connection non trivial is that when attempting it the user will run into the "db connection was accessed from a different thread" error, leading them to believe Flask's suggestion is the only correct approach.

Making the docs clearer should help guide those interested in the shared global connection approach towards setting check_same_thread to False.

A more drastic step would be to avoid raising the exception altogether if it would be safe to do so, but that would be a bit more complex.

And for even more transparency, here is the top-level code I am using to manage this in my database.py:

import sqlite3

# Ensure SQLite is in serialized mode, which makes it safe to set
# check_same_thread=False.
# This allows us to share a global connection to the database without causing
# "Objects created in a thread can only be used in that same thread" error with
# Flask ("Flask, as a WSGI application, uses one worker to handle one
# request/response cycle").
#
# Based on https://ricardoanderegg.com/posts/python-sqlite-thread-safety/
#
# Info on threadsafety attribute:
# https://docs.python.org/3/library/sqlite3.html#sqlite3.threadsafety
#
if sqlite3.threadsafety != 3:
    raise Exception(
        "SQLite is not in serialized mode (sqlite3.threadsafety != 3). " \
        "Cannot proceed."
    )

DATABASE = "db.sqlite3"

db = None

def init():
    _connect()
    _setup_tables()

def _connect():
    global db

    if db is None:
        db = sqlite3.connect(
            DATABASE,
            check_same_thread=False, # See above regarding sqlite3.threadsafety
            isolation_level=None # No implicit transactions
        ).cursor()

    # Get dictionaries instead of tuples.
    # https://flask.palletsprojects.com/en/2.2.x/patterns/sqlite3/#easy-querying
    # https://docs.python.org/3/library/sqlite3.html#row-objects
    db.row_factory = sqlite3.Row

def _setup_tables():
    ...

marcospgp and others added 5 commits January 30, 2023 10:48
@marcospgp
Update check_same_thread description in sqlite3.rst
6e68926 
Previous information was false: setting `check_same_thread` to `False` does not always require that the user ensure sequentialization themselves, depending on the value of `threadsafety`.
@marcospgp @erlend-aasland
Update Doc/library/sqlite3.rst
e0a3202 
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@marcospgp
Update sqlite3.rst
6ebe1ba
@marcospgp @CAM-Gerlach
Update Doc/library/sqlite3.rst
1aac9ce 
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@marcospgp @erlend-aasland
Update Doc/library/sqlite3.rst
6b865cf 
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
@erlend-aasland
Copy link
Contributor

Thanks again, @marcospgp, and thanks for chiming in CAM! I think this is an improvement over the current text. I also agree with CAM that some passages are a little bit vague. We can address those in seperate PRs.

@erlend-aasland erlend-aasland merged commit ee21110 into python:main Feb 1, 2023
@miss-islington
Copy link
Contributor

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

@bedevere-bot
Copy link

GH-101512 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Feb 1, 2023
@bedevere-bot
Copy link

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 1, 2023
@marcospgp @erlend-aasland
@CAM-Gerlach @miss-islington
Docs: improve accuracy of sqlite3 check_same_thread parameter (pyth…
63a5684 
…onGH-101351)

(cherry picked from commit ee21110)

Co-authored-by: Marcos Pereira <3464445+marcospgp@users.noreply.github.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@bedevere-bot bedevere-bot removed the needs backport to 3.10 only security fixes label Feb 1, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Feb 1, 2023
@marcospgp @erlend-aasland
@CAM-Gerlach @miss-islington
Docs: improve accuracy of sqlite3 check_same_thread parameter (pyth…
261276b 
…onGH-101351)

(cherry picked from commit ee21110)

Co-authored-by: Marcos Pereira <3464445+marcospgp@users.noreply.github.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
miss-islington added a commit that referenced this pull request Feb 2, 2023
@miss-islington @marcospgp
@erlend-aasland @CAM-Gerlach
Docs: improve accuracy of sqlite3 check_same_thread parameter (GH-1…
f8abe75 
…01351)

(cherry picked from commit ee21110)

Co-authored-by: Marcos Pereira <3464445+marcospgp@users.noreply.github.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
miss-islington added a commit that referenced this pull request Feb 2, 2023
@miss-islington @marcospgp
@erlend-aasland @CAM-Gerlach
Docs: improve accuracy of sqlite3 check_same_thread parameter (GH-1…
c2c970f 
…01351)

(cherry picked from commit ee21110)

Co-authored-by: Marcos Pereira <3464445+marcospgp@users.noreply.github.com>
Co-authored-by: Erlend E. Aasland <erlend.aasland@protonmail.com>
Co-authored-by: C.A.M. Gerlach <CAM.Gerlach@Gerlach.CAM>
@marcospgp marcospgp mentioned this pull request Feb 12, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erlend-aasland erlend-aasland erlend-aasland left review comments

@CAM-Gerlach CAM-Gerlach CAM-Gerlach left review comments

@berkerpeksag berkerpeksag Awaiting requested review from berkerpeksag berkerpeksag is a code owner

Assignees

@erlend-aasland erlend-aasland

Labels
docs Documentation in the Doc dir skip issue skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@marcospgp @erlend-aasland @miss-islington @bedevere-bot @CAM-Gerlach
0