Sqlite cursor garbage-collection issue with Python 3.11.0b3

[coleifer]

Bug report

This was reported to me on coleifer/peewee#2580 and, as I've managed to reproduce the issue, was asked by @erlend-aasland to submit a ticket here. Note that this issue does not manifest on other versions of Python (2.7, 3.6, 3.9 and 3.10) -- it appears to be a new issue on 3.11.0b3

The original reporter bisected and found the following commit introduced the regression: 3df0fc8

I believe this is a cursor garbage collection issue, though I may be mistaken. Keeping the cursors around in memory seems to cause the problem to manifest, but if you comment-out the line indicated below then the tests will pass. Note that Peewee doesn't do anything weird like keep the cursors in memory, this was just the first way I've been able to successfully reproduce the issue.

The test below does the following:

• create a 1st connection to a sqlite db and put some rows into it, committing changes
• create a 2nd connection to that db and verify we can see the rows, then delete them, committing changes
• begin a transaction in 1st connection, inserting 2 new rows
• verify the 2nd connection cannot see the uncommitted rows (this is where the failure occurs)
• verify the 1st connection can see its uncommitted rows, then commit
• lastly verify the 2nd connection can see the now-committed rows

What is doubly-confusing about this error is that, when the failure occurs, the SQL being executed should definitely NOT be returning None from the call to fetchone(). If the table did not exist, we would get a different error from Sqlite. If the table does exist and we just can't see any rows, then we should be getting 0 as the test asserts. Instead, the fetchone() is returning None.

Reproduce:

import glob
import os
import sqlite3

filename = '/tmp/test.db'
for f in glob.glob(filename + '*'):
    os.unlink(f)  # Cleanup anything from prev run(s).

CURSORS = {}

def sql(conn, sql, *params):
    curs = conn.cursor()
    curs.execute(sql, params)
    CURSORS[id(sql)] = curs  # COMMENT THIS OUT AND TEST WILL PASS.
    return curs

# Set up database w/some sample rows. Peewee sets isolation-level to None as we
# want to manage all transaction state ourselves, rather than use sqlite3's
# somewhat unusual semantics.
db = sqlite3.connect(filename, isolation_level=None)
db.execute('create table users (id integer not null primary key, '
           'username text not null)')
sql(db, 'insert into users (username) values (?), (?), (?)', 'u1', 'u2', 'u3')
db.commit()

# On 2nd connection verify rows are visible, then delete them.
new_db2 = sqlite3.connect(filename, isolation_level=None)
assert sql(new_db2, 'select count(*) from users').fetchone()[0] == 3
assert sql(new_db2, 'delete from users').rowcount == 3
new_db2.commit()

# Back in original connection, create 2 new users.
sql(db, 'begin')
sql(db, 'insert into users (username) values (?)', 'u4')
sql(db, 'insert into users (username) values (?)', 'u5')

# 2nd connection cannot see uncommitted changes.
# NOTE: this is the line that fails.
assert sql(new_db2, 'select count(*) from users').fetchone()[0] == 0

# Original conn can see its own changes.
assert sql(db, 'select count(*) from users').fetchone()[0] == 2
db.commit()

# Now the 2nd conn can see the changes.
assert sql(new_db2, 'select count(*) from users').fetchone()[0] == 2

On 3.11.0b3 the above fails on the indicated line with the following exception:

Traceback (most recent call last):
  File "/home/charles/tmp/py311/repro.py", line 40, in <module>
    assert sql(new_db2, 'select count(*) from users').fetchone()[0] == 0
           ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^
TypeError: 'NoneType' object is not subscriptable

Your environment

• CPython versions tested on: 2.7, 3.6, 3.9, 3.10 (passing), and 3.11.0b3 (failing)
• Sqlite3 version 3.35 and 3.38
• Linux / Debian stable

[erlend-aasland]

Thanks, again, for the report and investigation, @coleifer. FTR, I can reproduce this on macOS 12.2.1.

[erlend-aasland]

AFAICS (I might have missed something), this is what I believe happens¹ :

Preconditions

There exists a cursor cache. A new cursor is created and cached each time an SQL statement is executed. The cache key is the SQL string. If the same SQL query is executed, the new cursor will replace the existing cursor in the cache.

The problem

Cursor 1 executes the query select count(*) from t. Cursor 2 executes the same query (so it is reused from the LRU cache). When fetching the result from the second cursor, None is returned instead of a valid row.

Timeline (relevant bits)

Note: This all happens in the same connection.

1. Create a new cursor used for executing select count(*) from t
2. This (1.) implicitly creates a new sqlite3_stmt* for select count(*) from t; the new statement is now in the LRU cache.
3. The statement is stepped once in execute() and completed in fetchone().
4. Cursor is cached using the SQL string as key
5. Later, a new cursor is created for executing the same query (select count(*) from t)
   No new sqlite3_stmt* is created; the existing statement is fetched from the LRU cache and assigned to the cursor.
6. The statement is stepped once in execute(); it is now ready to return a result; data count is != 0.
7. The new cursor is put in the cache, replacing the previous cached cursor, since they use the same cache key.
   The old cursor has no references anymore, so it is deallocated; its "connected" statement is reset and cleared.
   Remember that the statement object is shared between the two cursors.
8. The new cursor executes fetchone(), but this fails, since the data count is now 0; the statement was recently reset.

The fix in #94042 works, because it clears the current statement from the cursor when the statement is stepped through via iternext. This means that in 7., there is no "connected" statement to reset; the cursor is deallocated, and nothing happens to the statement object in the new cursor.

Footnotes

1. sorry if it comes out a little bit confusing, I've only had time for one cup of coffee ☕ ↩

[erlend-aasland]

Another solution could be to modify cursor dealloc to only reset statements that are completed, but IIRC, that would (or could) break other corner cases. The best thing to do, imo, is to have the cursor clean up as soon as possible, and that is what #94042 does.

[erlend-aasland]

FTR, the repro does not need the custom transaction handling. Leaving isolation_level at default and removing the explicit BEGIN is ok.