Skip to content
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

[sqlite3] context manager leaves db locked if commit fails in __exit__ #71521

Closed
lciti mannequin opened this issue Jun 16, 2016 · 10 comments
Closed

[sqlite3] context manager leaves db locked if commit fails in __exit__ #71521

lciti mannequin opened this issue Jun 16, 2016 · 10 comments
Labels
3.9 only security fixes 3.10 only security fixes 3.11 only security fixes extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error

Comments

@lciti
Copy link
Mannequin

lciti mannequin commented Jun 16, 2016

BPO 27334
Nosy @vstinner, @ambv, @berkerpeksag, @lciti, @pablogsal, @erlend-aasland
PRs
  • bpo-27334: roll back transaction if sqlite3 context manager fails to commit #26202
  • bpo-27334: Fix ref. leak introduced by GH-26202 #27942
  • [3.10] bpo-27334: roll back transaction if sqlite3 context manager fails to commit (GH-26202) #27943
  • [3.9] bpo-27334: roll back transaction if sqlite3 context manager fails to commit (GH-26202) #27944
  • Files
  • fix_pysqlite_connection_exit.patch: Patch.
  • 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 = None
    closed_at = <Date 2021-08-28.18:41:36.848>
    created_at = <Date 2016-06-16.16:45:50.319>
    labels = ['extension-modules', 'type-bug', '3.9', '3.10', '3.11']
    title = '[sqlite3] context manager leaves db locked if commit fails in __exit__'
    updated_at = <Date 2021-08-31.22:06:54.828>
    user = 'https://github.com/lciti'

    bugs.python.org fields:

    activity = <Date 2021-08-31.22:06:54.828>
    actor = 'vstinner'
    assignee = 'none'
    closed = True
    closed_date = <Date 2021-08-28.18:41:36.848>
    closer = 'erlendaasland'
    components = ['Extension Modules']
    creation = <Date 2016-06-16.16:45:50.319>
    creator = 'lciti'
    dependencies = []
    files = ['43420']
    hgrepos = []
    issue_num = 27334
    keywords = ['patch']
    message_count = 10.0
    messages = ['268678', '268680', '393839', '393912', '400253', '400266', '400287', '400491', '400493', '400790']
    nosy_count = 7.0
    nosy_names = ['ghaering', 'vstinner', 'lukasz.langa', 'berker.peksag', 'lciti', 'pablogsal', 'erlendaasland']
    pr_nums = ['26202', '27942', '27943', '27944']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'behavior'
    url = 'https://bugs.python.org/issue27334'
    versions = ['Python 3.9', 'Python 3.10', 'Python 3.11']

    @lciti
    Copy link
    Mannequin Author

    lciti mannequin commented Jun 16, 2016

    I have reported this bug to the pysqlite module for python2 ( ghaering/pysqlite#103 ) but I also report it here because it applies to python3 too.

    The pysqlite3 context manager does not perform a rollback when a transaction fails because the database is locked by some other process performing non-DML statements (e.g. during the sqlite3 command line .dump method).

    To reproduce the problem, open a terminal and run the following:

    sqlite3 /tmp/test.db 'drop table person; create table person (id integer primary key, firstname varchar)'
    echo -e 'begin transaction;\nselect * from person;\n.system sleep 1000\nrollback;' | sqlite3 /tmp/test.db

    Leave this shell running and run the python3 interpreter from a different shell, then type:

    import sqlite3
    con = sqlite3.connect('/tmp/test.db')
    with con:                                                        
        con.execute("insert into person(firstname) values (?)", ("Jan",))
        pass

    You should receive the following:

          1 with con:
          2         con.execute("insert into person(firstname) values (?)", ("Jan",))
    ----> 3         pass
          4 
    
    OperationalError: database is locked
    

    Without exiting python, switch back to the first shell and kill the 'echo ... | sqlite3' process. Then run:

    sqlite3 /tmp/test.db .dump

    you should get:

    PRAGMA foreign_keys=OFF;
    BEGIN TRANSACTION;
    /**** ERROR: (5) database is locked *****/
    ROLLBACK; -- due to errors
    

    This means that the python process never executed a rollback and is still holding the lock. To release the lock one can exit python (clearly, this is not the intended behaviour of the context manager).

    I believe the reason for this problem is that the exception happened in the implicit commit that is run on exiting the context manager, rather than inside it. In fact the exception is in the pass line rather than in the execute line. This exception did not trigger a rollback because the it happened after pysqlite_connection_exit checks for exceptions.

    The expected behaviour (pysqlite3 rolling back and releasing the lock) is recovered if the initial blocking process is a Data Modification Language (DML) statement, e.g.:

    echo -e 'begin transaction; insert into person(firstname) values ("James");\n.system sleep 1000\nrollback;' | sqlite3 /tmp/test.db

    because this raises an exception at the execute time rather than at commit time.

    To fix this problem, I think the pysqlite_connection_exit function in src/connection.c should handle the case when the commit itself raises an exception, and invoke a rollback. Please see patch attached.

    @lciti lciti mannequin added the extension-modules C modules in the Modules dir label Jun 16, 2016
    @berkerpeksag
    Copy link
    Member

    You may also want to check that the method_name is "commit". A test case would nice to have.

    Note that the connection context manager will still fail cases like nested context managers. See bpo-16958.

    @berkerpeksag berkerpeksag added the type-bug An unexpected behavior, bug, or error label Jun 16, 2016
    @erlend-aasland
    Copy link
    Contributor

    In fact the exception is in the pass line rather than in the execute line.

    I can reproduce this without the pass line.

    I've taken the liberty to create a PR based on your patch, Luca. Berker's comments have been addressed in the PR.

    @erlend-aasland erlend-aasland added 3.9 only security fixes 3.10 only security fixes 3.11 only security fixes labels May 17, 2021
    @erlend-aasland erlend-aasland changed the title pysqlite3 context manager not performing rollback when a database is locked elsewhere for non-DML statements [sqlite3] context manager leaves db locked if commit fails in __exit__ May 17, 2021
    @erlend-aasland
    Copy link
    Contributor

    I believe the reason for this problem is that the exception happened in the
    implicit commit that is run on exiting the context manager, rather than
    inside it. In fact the exception is in the pass line rather than in the
    execute line. This exception did not trigger a rollback because the it
    happened after pysqlite_connection_exit checks for exceptions.

    FYI, here's the SQLite API interaction from the context manager, chronologically (using the test from the PR). (I only show the relevant arguments passed to the API, for readability.)

    sqlite3_prepare_v2("insert into t values('test')", insert_stmt) => SQLITE_OK
    sqlite3_get_autocommit()
    # Note, the insert statement is now prepared, but not executed yet.

    # Transaction control now begins
    sqlite3_prepare_v2("BEGIN ", begin_stmt) => SQLITE_OK
    sqlite3_step(begin_stmt) => SQLITE_DONE
    sqlite3_finalize(begin_stmt)

    # Here, the insert statement is executed
    sqlite3_bind_blob_parameter_count(insert_stmt)
    sqlite3_step(insert_stmt) => SQLITE_DONE
    sqlite3_changes()
    sqlite3_last_insert_rowid()
    sqlite3_reset(insert_stmt) => SQLITE_OK
    sqlite3_get_autocommit()

    # Enter __exit__: no exception has been raised, so it tries to commit
    sqlite3_prepare_v2("commit", commit_stmt) => SQLITE_OK
    sqlite3_step(commit_stmt) => SQLITE_BUSY (database is locked)
    sqlite3_finalize(commit_stmt)

    # After the fix, rollback is now executed
    sqlite3_prepare_v2("rollback", rollback_stmt)
    sqlite3_step(rollback_stmt) => SQLITE_DONE
    sqlite3_finalize(rollback_Stmt)

    As you can see, it does not fail (and raise an exception) until commit is issued inside __exit__.

    @pablogsal
    Copy link
    Member

    New changeset 7ecd342 by Erlend Egeberg Aasland in branch 'main':
    bpo-27334: roll back transaction if sqlite3 context manager fails to commit (GH-26202)
    7ecd342

    @ambv
    Copy link
    Contributor

    ambv commented Aug 25, 2021

    New changeset a3c11ce by Erlend Egeberg Aasland in branch 'main':
    bpo-27334: Fix reference leak introduced by #70390 (GH-27942)
    a3c11ce

    @pablogsal
    Copy link
    Member

    New changeset 52702e8 by Erlend Egeberg Aasland in branch '3.9':
    [3.9] bpo-27334: roll back transaction if sqlite3 context manager fails to commit (GH-26202) (GH-27944)
    52702e8

    @pablogsal
    Copy link
    Member

    New changeset 2a80893 by Erlend Egeberg Aasland in branch '3.10':
    [3.10] bpo-27334: roll back transaction if sqlite3 context manager fails to commit (GH-26202) (GH-27943)
    2a80893

    @erlend-aasland
    Copy link
    Contributor

    Thanks Luca, for the report, reproducer, and initial patch, Berker for helpful suggestion, and Łukasz, Pablo, & Victor for reviewing and merging.

    @vstinner
    Copy link
    Member

    Nice!

    @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
    3.9 only security fixes 3.10 only security fixes 3.11 only security fixes extension-modules C modules in the Modules dir type-bug An unexpected behavior, bug, or error
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants