From 2571aa6b0323a12755a2914f3bc36acde9648b88 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 31 Aug 2021 22:43:54 +0200 Subject: [PATCH 01/18] Clean up connection init, WIP --- Modules/_sqlite/connection.c | 113 ++++++++++++++++++++--------------- 1 file changed, 65 insertions(+), 48 deletions(-) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index bf803370c05719..d73aef3d690e51 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -58,15 +58,17 @@ static void _pysqlite_drop_unused_cursor_references(pysqlite_Connection* self); static void free_callback_context(callback_context *ctx); static void set_callback_context(callback_context **ctx_pp, callback_context *ctx); +static void connection_close(pysqlite_Connection *self); static PyObject * -new_statement_cache(pysqlite_Connection *self, int maxsize) +new_statement_cache(pysqlite_Connection *self, pysqlite_state *state, + int maxsize) { PyObject *args[] = { NULL, PyLong_FromLong(maxsize), }; if (args[1] == NULL) { return NULL; } - PyObject *lru_cache = self->state->lru_cache; + PyObject *lru_cache = state->lru_cache; size_t nargsf = 1 | PY_VECTORCALL_ARGUMENTS_OFFSET; PyObject *inner = PyObject_Vectorcall(lru_cache, args + 1, nargsf, NULL); Py_DECREF(args[1]); @@ -102,76 +104,63 @@ pysqlite_connection_init_impl(pysqlite_Connection *self, int cached_statements, int uri) /*[clinic end generated code: output=dc19df1c0e2b7b77 input=aa1f21bf12fe907a]*/ { - int rc; - if (PySys_Audit("sqlite3.connect", "O", database_obj) < 0) { + Py_DECREF(database_obj); // needed bco. the AC FSConverter return -1; } - pysqlite_state *state = pysqlite_get_state_by_type(Py_TYPE(self)); - self->state = state; + if (self->initialized) { + connection_close(self); + self->initialized = 0; + } + // Create and configure SQLite database object + sqlite3 *db; const char *database = PyBytes_AsString(database_obj); - - self->begin_statement = NULL; - - Py_CLEAR(self->statement_cache); - Py_CLEAR(self->cursors); - - Py_INCREF(Py_None); - Py_XSETREF(self->row_factory, Py_None); - - Py_INCREF(&PyUnicode_Type); - Py_XSETREF(self->text_factory, (PyObject*)&PyUnicode_Type); - + int rc; Py_BEGIN_ALLOW_THREADS - rc = sqlite3_open_v2(database, &self->db, + rc = sqlite3_open_v2(database, &db, SQLITE_OPEN_READWRITE | SQLITE_OPEN_CREATE | (uri ? SQLITE_OPEN_URI : 0), NULL); + if (rc == SQLITE_OK) { + (void)sqlite3_busy_timeout(db, (int)(timeout*1000)); + } Py_END_ALLOW_THREADS Py_DECREF(database_obj); // needed bco. the AC FSConverter + pysqlite_state *state = pysqlite_get_state_by_type(Py_TYPE(self)); if (rc != SQLITE_OK) { - _pysqlite_seterror(state, self->db); + _pysqlite_seterror(state, db); return -1; } - if (!isolation_level) { - isolation_level = PyUnicode_FromString(""); - if (!isolation_level) { - return -1; - } - } else { - Py_INCREF(isolation_level); - } - Py_CLEAR(self->isolation_level); - if (pysqlite_connection_set_isolation_level(self, isolation_level, NULL) != 0) { - Py_DECREF(isolation_level); - return -1; - } - Py_DECREF(isolation_level); - - self->statement_cache = new_statement_cache(self, cached_statements); - if (self->statement_cache == NULL) { - return -1; - } - if (PyErr_Occurred()) { + // Create cursor weak ref list and statement cache + PyObject *cursors = PyList_New(0); + if (cursors == NULL) { return -1; } - self->created_cursors = 0; - - /* Create list of weak references to cursors */ - self->cursors = PyList_New(0); - if (self->cursors == NULL) { + PyObject *cache = new_statement_cache(self, state, cached_statements); + if (cache == NULL) { + Py_DECREF(cursors); return -1; } + // Init connection state members + self->db = db; + self->state = state; self->detect_types = detect_types; - (void)sqlite3_busy_timeout(self->db, (int)(timeout*1000)); - self->thread_ident = PyThread_get_thread_ident(); + self->begin_statement = NULL; self->check_same_thread = check_same_thread; + self->thread_ident = PyThread_get_thread_ident(); + self->created_cursors = 0; + + Py_CLEAR(self->isolation_level); + Py_XSETREF(self->statement_cache, cache); + Py_XSETREF(self->cursors, cursors); + Py_XSETREF(self->row_factory, Py_NewRef(Py_None)); + Py_XSETREF(self->text_factory, Py_NewRef(&PyUnicode_Type)); set_callback_context(&self->trace_ctx, NULL); set_callback_context(&self->progress_ctx, NULL); @@ -188,12 +177,27 @@ pysqlite_connection_init_impl(pysqlite_Connection *self, self->ProgrammingError = state->ProgrammingError; self->NotSupportedError = state->NotSupportedError; + // Set isolation level at last, since it may trigger a COMMIT + if (isolation_level == NULL) { + isolation_level = PyUnicode_FromString(""); + if (isolation_level == NULL) { + return -1; + } + } + else { + Py_INCREF(isolation_level); + } + rc = pysqlite_connection_set_isolation_level(self, isolation_level, NULL); + Py_DECREF(isolation_level); + if (rc < 0) { + return -1; + } + if (PySys_Audit("sqlite3.connect/handle", "O", self) < 0) { return -1; } self->initialized = 1; - return 0; } @@ -264,10 +268,23 @@ connection_clear(pysqlite_Connection *self) return 0; } +static void +clear_sqlite_callbacks(pysqlite_Connection *self) +{ + (void)sqlite3_set_authorizer(self->db, NULL, NULL); + sqlite3_progress_handler(self->db, 0, NULL, NULL); +#ifdef HAVE_TRACE_V2 + sqlite3_trace_v2(self->db, SQLITE_TRACE_STMT, NULL, NULL); +#else + sqlite3_trace(self->db, NULL, NULL); +#endif +} + static void connection_close(pysqlite_Connection *self) { if (self->db) { + clear_sqlite_callbacks(self); int rc = sqlite3_close_v2(self->db); assert(rc == SQLITE_OK), (void)rc; self->db = NULL; From 101253ab39cb71b6e08e7c72b7276039f638f2c6 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 7 Sep 2021 21:58:42 +0200 Subject: [PATCH 02/18] Use AC to simplify reference handling in __init__ --- Modules/_sqlite/clinic/connection.c.h | 13 ++++--- Modules/_sqlite/connection.c | 49 ++++++++++++++++++++++----- 2 files changed, 49 insertions(+), 13 deletions(-) diff --git a/Modules/_sqlite/clinic/connection.c.h b/Modules/_sqlite/clinic/connection.c.h index 315d163dde668f..bf5a58d7756c9e 100644 --- a/Modules/_sqlite/clinic/connection.c.h +++ b/Modules/_sqlite/clinic/connection.c.h @@ -4,7 +4,7 @@ preserve static int pysqlite_connection_init_impl(pysqlite_Connection *self, - PyObject *database_obj, double timeout, + const char *database, double timeout, int detect_types, PyObject *isolation_level, int check_same_thread, PyObject *factory, int cached_statements, int uri); @@ -19,7 +19,7 @@ pysqlite_connection_init(PyObject *self, PyObject *args, PyObject *kwargs) PyObject * const *fastargs; Py_ssize_t nargs = PyTuple_GET_SIZE(args); Py_ssize_t noptargs = nargs + (kwargs ? PyDict_GET_SIZE(kwargs) : 0) - 1; - PyObject *database_obj; + const char *database = NULL; double timeout = 5.0; int detect_types = 0; PyObject *isolation_level = NULL; @@ -32,7 +32,7 @@ pysqlite_connection_init(PyObject *self, PyObject *args, PyObject *kwargs) if (!fastargs) { goto exit; } - if (!PyUnicode_FSConverter(fastargs[0], &database_obj)) { + if (!clinic_fsconverter(fastargs[0], &database)) { goto exit; } if (!noptargs) { @@ -97,9 +97,12 @@ pysqlite_connection_init(PyObject *self, PyObject *args, PyObject *kwargs) goto exit; } skip_optional_pos: - return_value = pysqlite_connection_init_impl((pysqlite_Connection *)self, database_obj, timeout, detect_types, isolation_level, check_same_thread, factory, cached_statements, uri); + return_value = pysqlite_connection_init_impl((pysqlite_Connection *)self, database, timeout, detect_types, isolation_level, check_same_thread, factory, cached_statements, uri); exit: + /* Cleanup for database */ + PyMem_Free((void *)database); + return return_value; } @@ -816,4 +819,4 @@ pysqlite_connection_exit(pysqlite_Connection *self, PyObject *const *args, Py_ss #ifndef PYSQLITE_CONNECTION_LOAD_EXTENSION_METHODDEF #define PYSQLITE_CONNECTION_LOAD_EXTENSION_METHODDEF #endif /* !defined(PYSQLITE_CONNECTION_LOAD_EXTENSION_METHODDEF) */ -/*[clinic end generated code: output=9c0dfc6c1ebf9039 input=a9049054013a1b77]*/ +/*[clinic end generated code: output=5b7268875f33c016 input=a9049054013a1b77]*/ diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index d73aef3d690e51..8e1ebb8d531226 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -33,6 +33,32 @@ #define HAVE_TRACE_V2 #endif +static int +clinic_fsconverter(PyObject *pathlike, const char **result) +{ + PyObject *bytes = NULL; + Py_ssize_t len; + char *str; + + if (!PyUnicode_FSConverter(pathlike, &bytes)) { + goto error; + } + else if (PyBytes_AsStringAndSize(bytes, &str, &len) < 0) { + goto error; + } + else if ((*result = PyMem_Malloc(len))) { + goto error; + } + + memcpy((void *)(*result), str, len); + Py_DECREF(bytes); + return 1; + +error: + Py_XDECREF(bytes); + return 0; +} + #define clinic_state() (pysqlite_get_state(NULL)) #include "clinic/connection.c.h" #undef clinic_state @@ -83,10 +109,21 @@ new_statement_cache(pysqlite_Connection *self, pysqlite_state *state, return res; } +/*[python input] +class FSConverter_converter(CConverter): + type = "const char *" + converter = "clinic_fsconverter" + def converter_init(self): + self.c_default = "NULL" + def cleanup(self): + return f"PyMem_Free((void *){self.name});\n" +[python start generated code]*/ +/*[python end generated code: output=da39a3ee5e6b4b0d input=7b3be538bc4058c0]*/ + /*[clinic input] _sqlite3.Connection.__init__ as pysqlite_connection_init - database as database_obj: object(converter='PyUnicode_FSConverter') + database: FSConverter timeout: double = 5.0 detect_types: int = 0 isolation_level: object = NULL @@ -98,14 +135,13 @@ _sqlite3.Connection.__init__ as pysqlite_connection_init static int pysqlite_connection_init_impl(pysqlite_Connection *self, - PyObject *database_obj, double timeout, + const char *database, double timeout, int detect_types, PyObject *isolation_level, int check_same_thread, PyObject *factory, int cached_statements, int uri) -/*[clinic end generated code: output=dc19df1c0e2b7b77 input=aa1f21bf12fe907a]*/ +/*[clinic end generated code: output=bc39e55eb0b68783 input=f8d1f7efc0d84104]*/ { - if (PySys_Audit("sqlite3.connect", "O", database_obj) < 0) { - Py_DECREF(database_obj); // needed bco. the AC FSConverter + if (PySys_Audit("sqlite3.connect", "s", database) < 0) { return -1; } @@ -116,7 +152,6 @@ pysqlite_connection_init_impl(pysqlite_Connection *self, // Create and configure SQLite database object sqlite3 *db; - const char *database = PyBytes_AsString(database_obj); int rc; Py_BEGIN_ALLOW_THREADS rc = sqlite3_open_v2(database, &db, @@ -127,8 +162,6 @@ pysqlite_connection_init_impl(pysqlite_Connection *self, } Py_END_ALLOW_THREADS - Py_DECREF(database_obj); // needed bco. the AC FSConverter - pysqlite_state *state = pysqlite_get_state_by_type(Py_TYPE(self)); if (rc != SQLITE_OK) { _pysqlite_seterror(state, db); From c3efc544427722dbfce6d7068d21707c2579e9fe Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 7 Sep 2021 22:08:09 +0200 Subject: [PATCH 03/18] Clear object if reinitialising --- Modules/_sqlite/connection.c | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index 8e1ebb8d531226..4ced9f5c783196 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -147,6 +147,8 @@ pysqlite_connection_init_impl(pysqlite_Connection *self, if (self->initialized) { connection_close(self); + PyTypeObject *tp = Py_TYPE(self); + tp->tp_clear((PyObject *)self); self->initialized = 0; } @@ -184,20 +186,18 @@ pysqlite_connection_init_impl(pysqlite_Connection *self, self->db = db; self->state = state; self->detect_types = detect_types; + self->isolation_level = NULL; self->begin_statement = NULL; self->check_same_thread = check_same_thread; self->thread_ident = PyThread_get_thread_ident(); + self->statement_cache = cache; + self->cursors = cursors; self->created_cursors = 0; - - Py_CLEAR(self->isolation_level); - Py_XSETREF(self->statement_cache, cache); - Py_XSETREF(self->cursors, cursors); - Py_XSETREF(self->row_factory, Py_NewRef(Py_None)); - Py_XSETREF(self->text_factory, Py_NewRef(&PyUnicode_Type)); - - set_callback_context(&self->trace_ctx, NULL); - set_callback_context(&self->progress_ctx, NULL); - set_callback_context(&self->authorizer_ctx, NULL); + self->row_factory = Py_NewRef(Py_None); + self->text_factory = Py_NewRef(&PyUnicode_Type); + self->trace_ctx = NULL; + self->progress_ctx = NULL; + self->authorizer_ctx = NULL; self->Warning = state->Warning; self->Error = state->Error; From 93fb30a08e8ab4d7862203421b3d34a321e417d5 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 7 Sep 2021 22:22:15 +0200 Subject: [PATCH 04/18] Make room for and include null terminator --- Modules/_sqlite/connection.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index 4ced9f5c783196..c6729c3208ac57 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -46,11 +46,11 @@ clinic_fsconverter(PyObject *pathlike, const char **result) else if (PyBytes_AsStringAndSize(bytes, &str, &len) < 0) { goto error; } - else if ((*result = PyMem_Malloc(len))) { + else if ((*result = (const char *)PyMem_Malloc(len+1)) == NULL) { goto error; } - memcpy((void *)(*result), str, len); + memcpy((void *)(*result), str, len+1); Py_DECREF(bytes); return 1; From 0581d1f30efda318406a1744d16a5aa4bf7abe6b Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 7 Sep 2021 22:24:01 +0200 Subject: [PATCH 05/18] Allow threads while closing database --- Modules/_sqlite/connection.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index c6729c3208ac57..5b25377e7a2068 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -318,7 +318,12 @@ connection_close(pysqlite_Connection *self) { if (self->db) { clear_sqlite_callbacks(self); - int rc = sqlite3_close_v2(self->db); + + int rc; + Py_BEGIN_ALLOW_THREADS + rc = sqlite3_close_v2(self->db); + Py_END_ALLOW_THREADS + assert(rc == SQLITE_OK), (void)rc; self->db = NULL; } From ff81aff9a24d27daf92ea2b96d6d21dae778eefe Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 7 Sep 2021 22:33:27 +0200 Subject: [PATCH 06/18] Add borrowed refs comment --- Modules/_sqlite/connection.c | 1 + 1 file changed, 1 insertion(+) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index 5b25377e7a2068..4063a585931440 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -199,6 +199,7 @@ pysqlite_connection_init_impl(pysqlite_Connection *self, self->progress_ctx = NULL; self->authorizer_ctx = NULL; + // Borrowed refs self->Warning = state->Warning; self->Error = state->Error; self->InterfaceError = state->InterfaceError; From c30f06b458c8113e1df64b5c9c905c1e9e8615eb Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 7 Sep 2021 22:47:11 +0200 Subject: [PATCH 07/18] Improve naming: cached_statements => cache_size --- Modules/_sqlite/clinic/connection.c.h | 12 ++++++------ Modules/_sqlite/connection.c | 8 ++++---- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/Modules/_sqlite/clinic/connection.c.h b/Modules/_sqlite/clinic/connection.c.h index bf5a58d7756c9e..a7a65926bfa4fd 100644 --- a/Modules/_sqlite/clinic/connection.c.h +++ b/Modules/_sqlite/clinic/connection.c.h @@ -7,7 +7,7 @@ pysqlite_connection_init_impl(pysqlite_Connection *self, const char *database, double timeout, int detect_types, PyObject *isolation_level, int check_same_thread, PyObject *factory, - int cached_statements, int uri); + int cache_size, int uri); static int pysqlite_connection_init(PyObject *self, PyObject *args, PyObject *kwargs) @@ -25,7 +25,7 @@ pysqlite_connection_init(PyObject *self, PyObject *args, PyObject *kwargs) PyObject *isolation_level = NULL; int check_same_thread = 1; PyObject *factory = (PyObject*)clinic_state()->ConnectionType; - int cached_statements = 128; + int cache_size = 128; int uri = 0; fastargs = _PyArg_UnpackKeywords(_PyTuple_CAST(args)->ob_item, nargs, kwargs, NULL, &_parser, 1, 8, 0, argsbuf); @@ -84,8 +84,8 @@ pysqlite_connection_init(PyObject *self, PyObject *args, PyObject *kwargs) } } if (fastargs[6]) { - cached_statements = _PyLong_AsInt(fastargs[6]); - if (cached_statements == -1 && PyErr_Occurred()) { + cache_size = _PyLong_AsInt(fastargs[6]); + if (cache_size == -1 && PyErr_Occurred()) { goto exit; } if (!--noptargs) { @@ -97,7 +97,7 @@ pysqlite_connection_init(PyObject *self, PyObject *args, PyObject *kwargs) goto exit; } skip_optional_pos: - return_value = pysqlite_connection_init_impl((pysqlite_Connection *)self, database, timeout, detect_types, isolation_level, check_same_thread, factory, cached_statements, uri); + return_value = pysqlite_connection_init_impl((pysqlite_Connection *)self, database, timeout, detect_types, isolation_level, check_same_thread, factory, cache_size, uri); exit: /* Cleanup for database */ @@ -819,4 +819,4 @@ pysqlite_connection_exit(pysqlite_Connection *self, PyObject *const *args, Py_ss #ifndef PYSQLITE_CONNECTION_LOAD_EXTENSION_METHODDEF #define PYSQLITE_CONNECTION_LOAD_EXTENSION_METHODDEF #endif /* !defined(PYSQLITE_CONNECTION_LOAD_EXTENSION_METHODDEF) */ -/*[clinic end generated code: output=5b7268875f33c016 input=a9049054013a1b77]*/ +/*[clinic end generated code: output=dd10b1f80c3ba3d3 input=a9049054013a1b77]*/ diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index 4063a585931440..4ef5b6025be944 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -129,7 +129,7 @@ _sqlite3.Connection.__init__ as pysqlite_connection_init isolation_level: object = NULL check_same_thread: bool(accept={int}) = True factory: object(c_default='(PyObject*)clinic_state()->ConnectionType') = ConnectionType - cached_statements: int = 128 + cached_statements as cache_size: int = 128 uri: bool = False [clinic start generated code]*/ @@ -138,8 +138,8 @@ pysqlite_connection_init_impl(pysqlite_Connection *self, const char *database, double timeout, int detect_types, PyObject *isolation_level, int check_same_thread, PyObject *factory, - int cached_statements, int uri) -/*[clinic end generated code: output=bc39e55eb0b68783 input=f8d1f7efc0d84104]*/ + int cache_size, int uri) +/*[clinic end generated code: output=ded53c0781f8fb58 input=3588bcf7e2987e6a]*/ { if (PySys_Audit("sqlite3.connect", "s", database) < 0) { return -1; @@ -176,7 +176,7 @@ pysqlite_connection_init_impl(pysqlite_Connection *self, return -1; } - PyObject *cache = new_statement_cache(self, state, cached_statements); + PyObject *cache = new_statement_cache(self, state, cache_size); if (cache == NULL) { Py_DECREF(cursors); return -1; From 401a1f0c34f935b13854550b4cea660a2416145c Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 7 Sep 2021 23:12:11 +0200 Subject: [PATCH 08/18] Add tests --- Lib/sqlite3/test/dbapi.py | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/Lib/sqlite3/test/dbapi.py b/Lib/sqlite3/test/dbapi.py index 89f773daf24a16..a58d38e37dfa0f 100644 --- a/Lib/sqlite3/test/dbapi.py +++ b/Lib/sqlite3/test/dbapi.py @@ -331,6 +331,21 @@ def test_drop_unused_refs(self): cu = self.cx.execute(f"select {n}") self.assertEqual(cu.fetchone()[0], n) + def test_connection_reinit(self): + db = ":memory:" + cx = sqlite.connect(db) + cx.__init__(db) + + def test_connection_bad_reinit(self): + cx = sqlite.connect(":memory:") + with temp_dir() as db: + try: + cx.__init__(db) + except sqlite.OperationalError: + pass + else: + self.fail("Unexpected success") + class UninitialisedConnectionTests(unittest.TestCase): def setUp(self): From 6d684b4ea477547912781a6c411a0cb39368b99e Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 7 Sep 2021 23:13:13 +0200 Subject: [PATCH 09/18] Adjustments --- Modules/_sqlite/connection.c | 25 ++++++++----------------- 1 file changed, 8 insertions(+), 17 deletions(-) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index 4ef5b6025be944..973a3545959878 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -146,9 +146,9 @@ pysqlite_connection_init_impl(pysqlite_Connection *self, } if (self->initialized) { - connection_close(self); PyTypeObject *tp = Py_TYPE(self); tp->tp_clear((PyObject *)self); + connection_close(self); self->initialized = 0; } @@ -170,18 +170,6 @@ pysqlite_connection_init_impl(pysqlite_Connection *self, return -1; } - // Create cursor weak ref list and statement cache - PyObject *cursors = PyList_New(0); - if (cursors == NULL) { - return -1; - } - - PyObject *cache = new_statement_cache(self, state, cache_size); - if (cache == NULL) { - Py_DECREF(cursors); - return -1; - } - // Init connection state members self->db = db; self->state = state; @@ -190,8 +178,8 @@ pysqlite_connection_init_impl(pysqlite_Connection *self, self->begin_statement = NULL; self->check_same_thread = check_same_thread; self->thread_ident = PyThread_get_thread_ident(); - self->statement_cache = cache; - self->cursors = cursors; + self->statement_cache = new_statement_cache(self, state, cache_size);; + self->cursors = PyList_New(0); self->created_cursors = 0; self->row_factory = Py_NewRef(Py_None); self->text_factory = Py_NewRef(&PyUnicode_Type); @@ -211,6 +199,10 @@ pysqlite_connection_init_impl(pysqlite_Connection *self, self->ProgrammingError = state->ProgrammingError; self->NotSupportedError = state->NotSupportedError; + if (self->cursors == NULL || self->statement_cache == NULL) { + return -1; + } + // Set isolation level at last, since it may trigger a COMMIT if (isolation_level == NULL) { isolation_level = PyUnicode_FromString(""); @@ -318,10 +310,9 @@ static void connection_close(pysqlite_Connection *self) { if (self->db) { - clear_sqlite_callbacks(self); - int rc; Py_BEGIN_ALLOW_THREADS + clear_sqlite_callbacks(self); rc = sqlite3_close_v2(self->db); Py_END_ALLOW_THREADS From 4f55bb85d7a5f0d620c459d3a769cc16582f3309 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 7 Sep 2021 23:31:33 +0200 Subject: [PATCH 10/18] Upgrade "bad reinit" test to include Petr's reproducer --- Lib/sqlite3/test/dbapi.py | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/Lib/sqlite3/test/dbapi.py b/Lib/sqlite3/test/dbapi.py index a58d38e37dfa0f..eac3b2b36751ea 100644 --- a/Lib/sqlite3/test/dbapi.py +++ b/Lib/sqlite3/test/dbapi.py @@ -338,13 +338,16 @@ def test_connection_reinit(self): def test_connection_bad_reinit(self): cx = sqlite.connect(":memory:") + with cx: + cx.execute("create table t(t)") with temp_dir() as db: - try: - cx.__init__(db) - except sqlite.OperationalError: - pass - else: - self.fail("Unexpected success") + self.assertRaisesRegex(sqlite.OperationalError, + "unable to open database file", + cx.__init__, db) + self.assertRaisesRegex(sqlite.ProgrammingError, + "Base Connection.__init__ not called", + cx.executemany, "insert into t values(?)", + ((v,) for v in range(3))) class UninitialisedConnectionTests(unittest.TestCase): From 87903ff64db99ef7203bb40a02012057d4eeb702 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Tue, 7 Sep 2021 23:39:17 +0200 Subject: [PATCH 11/18] Always free callback contexts after closing database --- Modules/_sqlite/connection.c | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index 973a3545959878..f38a3bb1fbbf9f 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -306,6 +306,14 @@ clear_sqlite_callbacks(pysqlite_Connection *self) #endif } +static void +free_callback_contexts(pysqlite_Connection *self) +{ + set_callback_context(&self->trace_ctx, NULL); + set_callback_context(&self->progress_ctx, NULL); + set_callback_context(&self->authorizer_ctx, NULL); +} + static void connection_close(pysqlite_Connection *self) { @@ -315,20 +323,13 @@ connection_close(pysqlite_Connection *self) clear_sqlite_callbacks(self); rc = sqlite3_close_v2(self->db); Py_END_ALLOW_THREADS + free_callback_contexts(self); assert(rc == SQLITE_OK), (void)rc; self->db = NULL; } } -static void -free_callback_contexts(pysqlite_Connection *self) -{ - set_callback_context(&self->trace_ctx, NULL); - set_callback_context(&self->progress_ctx, NULL); - set_callback_context(&self->authorizer_ctx, NULL); -} - static void connection_dealloc(pysqlite_Connection *self) { @@ -338,7 +339,6 @@ connection_dealloc(pysqlite_Connection *self) /* Clean up if user has not called .close() explicitly. */ connection_close(self); - free_callback_contexts(self); tp->tp_free(self); Py_DECREF(tp); From 73c200604fabea9dbb76ce2fb0e479c7f040497a Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Wed, 8 Sep 2021 10:48:01 +0200 Subject: [PATCH 12/18] Improve "good" reinit test --- Lib/sqlite3/test/dbapi.py | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/Lib/sqlite3/test/dbapi.py b/Lib/sqlite3/test/dbapi.py index eac3b2b36751ea..be9deec59376e3 100644 --- a/Lib/sqlite3/test/dbapi.py +++ b/Lib/sqlite3/test/dbapi.py @@ -334,7 +334,27 @@ def test_drop_unused_refs(self): def test_connection_reinit(self): db = ":memory:" cx = sqlite.connect(db) + cx.text_factory = bytes + cx.row_factory = sqlite.Row + cu = cx.cursor() + cu.execute("create table foo (bar)") + cu.executemany("insert into foo (bar) values (?)", + ((str(v),) for v in range(4))) + cu.execute("select bar from foo") + + rows = [r for r in cu.fetchmany(2)] + self.assertTrue(all(isinstance(r, sqlite.Row) for r in rows)) + self.assertEqual([r[0] for r in rows], [b"0", b"1"]) + cx.__init__(db) + cx.execute("create table foo (bar)") + cx.executemany("insert into foo (bar) values (?)", + ((v,) for v in ("a", "b", "c", "d"))) + + # This uses the old database, old row factory, but new text factory + rows = [r for r in cu.fetchall()] + self.assertTrue(all(isinstance(r, sqlite.Row) for r in rows)) + self.assertEqual([r[0] for r in rows], ["2", "3"]) def test_connection_bad_reinit(self): cx = sqlite.connect(":memory:") From 4b064437b899921ce302ea46e23418b6cdb72b29 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Wed, 8 Sep 2021 11:00:26 +0200 Subject: [PATCH 13/18] Deprecate reinitialisation --- Lib/sqlite3/test/dbapi.py | 13 +++++++++---- Modules/_sqlite/connection.c | 7 +++++++ 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/Lib/sqlite3/test/dbapi.py b/Lib/sqlite3/test/dbapi.py index be9deec59376e3..7fddebfa7da62b 100644 --- a/Lib/sqlite3/test/dbapi.py +++ b/Lib/sqlite3/test/dbapi.py @@ -346,7 +346,10 @@ def test_connection_reinit(self): self.assertTrue(all(isinstance(r, sqlite.Row) for r in rows)) self.assertEqual([r[0] for r in rows], [b"0", b"1"]) - cx.__init__(db) + with self.assertWarns(DeprecationWarning) as cm: + cx.__init__(db) + self.assertIn("dbapi.py", cm.filename) + cx.execute("create table foo (bar)") cx.executemany("insert into foo (bar) values (?)", ((v,) for v in ("a", "b", "c", "d"))) @@ -361,9 +364,11 @@ def test_connection_bad_reinit(self): with cx: cx.execute("create table t(t)") with temp_dir() as db: - self.assertRaisesRegex(sqlite.OperationalError, - "unable to open database file", - cx.__init__, db) + with self.assertWarns(DeprecationWarning) as cm: + msg = "unable to open database file" + with self.assertRaisesRegex(sqlite.OperationalError, msg): + cx.__init__(db) + self.assertIn("dbapi.py", cm.filename) self.assertRaisesRegex(sqlite.ProgrammingError, "Base Connection.__init__ not called", cx.executemany, "insert into t values(?)", diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index f38a3bb1fbbf9f..2301ed600a91fd 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -146,6 +146,13 @@ pysqlite_connection_init_impl(pysqlite_Connection *self, } if (self->initialized) { + int rc = PyErr_WarnEx(PyExc_DeprecationWarning, + "Connection reinitialization is deprecated and " + "will be removed in Python 3.13", + 1); + if (rc < 0) { + return -1; + } PyTypeObject *tp = Py_TYPE(self); tp->tp_clear((PyObject *)self); connection_close(self); From cec8f717f7d62e0026e36f0adcd676883dd1ac32 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Wed, 8 Sep 2021 11:24:46 +0200 Subject: [PATCH 14/18] SQLite will clear callbacks when closing; remove explicit close --- Modules/_sqlite/connection.c | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index 2301ed600a91fd..3bc7c1d5e73e26 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -301,18 +301,6 @@ connection_clear(pysqlite_Connection *self) return 0; } -static void -clear_sqlite_callbacks(pysqlite_Connection *self) -{ - (void)sqlite3_set_authorizer(self->db, NULL, NULL); - sqlite3_progress_handler(self->db, 0, NULL, NULL); -#ifdef HAVE_TRACE_V2 - sqlite3_trace_v2(self->db, SQLITE_TRACE_STMT, NULL, NULL); -#else - sqlite3_trace(self->db, NULL, NULL); -#endif -} - static void free_callback_contexts(pysqlite_Connection *self) { @@ -327,7 +315,6 @@ connection_close(pysqlite_Connection *self) if (self->db) { int rc; Py_BEGIN_ALLOW_THREADS - clear_sqlite_callbacks(self); rc = sqlite3_close_v2(self->db); Py_END_ALLOW_THREADS free_callback_contexts(self); From cb96a4064f6de518b46716b48047fee9dafeebe1 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Fri, 10 Sep 2021 06:29:42 +0200 Subject: [PATCH 15/18] Revert "Deprecate reinitialisation" This reverts commit 4b064437b899921ce302ea46e23418b6cdb72b29. --- Lib/sqlite3/test/dbapi.py | 13 ++++--------- Modules/_sqlite/connection.c | 7 ------- 2 files changed, 4 insertions(+), 16 deletions(-) diff --git a/Lib/sqlite3/test/dbapi.py b/Lib/sqlite3/test/dbapi.py index 7fddebfa7da62b..be9deec59376e3 100644 --- a/Lib/sqlite3/test/dbapi.py +++ b/Lib/sqlite3/test/dbapi.py @@ -346,10 +346,7 @@ def test_connection_reinit(self): self.assertTrue(all(isinstance(r, sqlite.Row) for r in rows)) self.assertEqual([r[0] for r in rows], [b"0", b"1"]) - with self.assertWarns(DeprecationWarning) as cm: - cx.__init__(db) - self.assertIn("dbapi.py", cm.filename) - + cx.__init__(db) cx.execute("create table foo (bar)") cx.executemany("insert into foo (bar) values (?)", ((v,) for v in ("a", "b", "c", "d"))) @@ -364,11 +361,9 @@ def test_connection_bad_reinit(self): with cx: cx.execute("create table t(t)") with temp_dir() as db: - with self.assertWarns(DeprecationWarning) as cm: - msg = "unable to open database file" - with self.assertRaisesRegex(sqlite.OperationalError, msg): - cx.__init__(db) - self.assertIn("dbapi.py", cm.filename) + self.assertRaisesRegex(sqlite.OperationalError, + "unable to open database file", + cx.__init__, db) self.assertRaisesRegex(sqlite.ProgrammingError, "Base Connection.__init__ not called", cx.executemany, "insert into t values(?)", diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index 3bc7c1d5e73e26..59091f25c4776d 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -146,13 +146,6 @@ pysqlite_connection_init_impl(pysqlite_Connection *self, } if (self->initialized) { - int rc = PyErr_WarnEx(PyExc_DeprecationWarning, - "Connection reinitialization is deprecated and " - "will be removed in Python 3.13", - 1); - if (rc < 0) { - return -1; - } PyTypeObject *tp = Py_TYPE(self); tp->tp_clear((PyObject *)self); connection_close(self); From f3a1f39c5e993bc79437b56628640ae4128b3718 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 18 Oct 2021 14:02:28 +0200 Subject: [PATCH 16/18] Clear database pointer before calling sqlite3_close_v2() --- Modules/_sqlite/connection.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index e10c8c9849a7d0..8d80bcaeb5460f 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -306,14 +306,15 @@ static void connection_close(pysqlite_Connection *self) { if (self->db) { - int rc; - Py_BEGIN_ALLOW_THREADS - rc = sqlite3_close_v2(self->db); - Py_END_ALLOW_THREADS free_callback_contexts(self); - assert(rc == SQLITE_OK), (void)rc; + sqlite3 *db = self->db; self->db = NULL; + + Py_BEGIN_ALLOW_THREADS + int rc = sqlite3_close_v2(db); + assert(rc == SQLITE_OK), (void)rc; + Py_END_ALLOW_THREADS } } From 82e1f5ebbfa1abe139d1296bff5f166cf9e48325 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 15 Nov 2021 10:36:05 +0100 Subject: [PATCH 17/18] Fix merge --- Modules/_sqlite/connection.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index 71af9b82430ca6..a5d76e394e717e 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -208,7 +208,7 @@ pysqlite_connection_init_impl(pysqlite_Connection *self, } // Create LRU statement cache; returns a new reference. - PyObject *statement_cache = new_statement_cache(self, cached_statements); + PyObject *statement_cache = new_statement_cache(self, state, cache_size); if (statement_cache == NULL) { return -1; } From 83f67368a69fe069f94735e27c14c38004ddd8c5 Mon Sep 17 00:00:00 2001 From: "Erlend E. Aasland" Date: Mon, 15 Nov 2021 10:50:13 +0100 Subject: [PATCH 18/18] bpo-45512: Raise sqlite3.Connection.__init__ is called with bad isolation level --- Lib/test/test_sqlite3/test_dbapi.py | 12 ++++++++++-- Modules/_sqlite/connection.c | 6 +++--- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/Lib/test/test_sqlite3/test_dbapi.py b/Lib/test/test_sqlite3/test_dbapi.py index 3055b6d295cf87..4b387cb952455a 100644 --- a/Lib/test/test_sqlite3/test_dbapi.py +++ b/Lib/test/test_sqlite3/test_dbapi.py @@ -48,8 +48,8 @@ def managed_connect(*args, in_mem=False, **kwargs): # Helper for temporary memory databases -def memory_database(): - cx = sqlite.connect(":memory:") +def memory_database(*args, **kwargs): + cx = sqlite.connect(":memory:", *args, **kwargs) return contextlib.closing(cx) @@ -547,6 +547,14 @@ def test_connection_bad_reinit(self): cx.executemany, "insert into t values(?)", ((v,) for v in range(3))) + def test_connection_init_bad_isolation_level(self): + msg = ( + "isolation_level string must be '', 'DEFERRED', 'IMMEDIATE', or " + "'EXCLUSIVE'" + ) + with self.assertRaisesRegex(ValueError, msg): + memory_database(isolation_level="BOGUS") + class UninitialisedConnectionTests(unittest.TestCase): def setUp(self): diff --git a/Modules/_sqlite/connection.c b/Modules/_sqlite/connection.c index a5d76e394e717e..e7947671db3544 100644 --- a/Modules/_sqlite/connection.c +++ b/Modules/_sqlite/connection.c @@ -129,6 +129,9 @@ get_begin_statement(const char *level) return begin_statements[i]; } } + PyErr_SetString(PyExc_ValueError, + "isolation_level string must be '', 'DEFERRED', " + "'IMMEDIATE', or 'EXCLUSIVE'"); return NULL; } @@ -1399,9 +1402,6 @@ pysqlite_connection_set_isolation_level(pysqlite_Connection* self, PyObject* iso } const char *stmt = get_begin_statement(cstr_level); if (stmt == NULL) { - PyErr_SetString(PyExc_ValueError, - "isolation_level string must be '', 'DEFERRED', " - "'IMMEDIATE', or 'EXCLUSIVE'"); return -1; } self->begin_statement = stmt;