[Trac-tickets] [The Trac Project] #2196: Trac can't use the thread-safe version of sqlite

[The Trac Project]

#2196: Trac can't use the thread-safe version of sqlite
---------------------+------------------------------------------------------
 Reporter:  cboos    |       Owner:  jonas
     Type:  defect   |      Status:  new  
 Priority:  normal   |   Milestone:       
Component:  general  |     Version:  devel
 Severity:  normal   |   Keywords:           |  
---------------------+------------------------------------------------------
 If SQLite has been configured with the `--enable-threadsafe` switch,
 the resulting binary '''won't''' work with Trac:

 {{{
 Oops...

 Trac detected an internal error:

 library routine called out of sequence
 Traceback (most recent call last):
   File "/opt/trac/stable-bct-trac/lib/python2.3/site-
 packages/trac/web/standalone.py", line 235, in _do_trac_req
     dispatch_request(path_info, req, env)
 ...
   File "/usr/lib/python2.3/site-packages/sqlite/main.py", line 244, in
 execute
     self.rs = self.con.db.execute(SQL)
 ProgrammingError: library routine called out of sequence
 }}}


 This is because the `--enable-threadsafe` flag will enable,
 at the C level, a check for thread consistency.
 That check enforces that each operation involving a sqlite3* object
 will be performed from within the same thread that created that
 object, or a `SQLITE_MISUSE` error will be returned.

 This is the same kind of check that PySQLite does when the
 `check_same_thread=True` flag is given to the `Connection`
 constructor.

 Currently in Trac, we explicitely disable that check at the PySQLite
 level, but there's no way to disable it on the SQLite binary if it
 has been compiled with `--enable-threadsafe`.

 The workaround is to compile SQLite without that flag,
 which, fortunately for us, is the default on Linux.

 But it's not intuitive, as one would expect to turn thread safety
 ''on'' for a library which will be used in a multi-threaded context.

 The real question is: can we assume that using SQLite that
 way is always safe? For now, it seems to be the case, but
 I guess there were reasons for having those checks in the first
 place.

 In #1811, cmlenz says that it's OK to do what we do,
 but from the [http://sqlite.org/faq.html#q8 SQLite FAQ] which is
 cited there, I read:
   ''It is never safe to use the same sqlite3 structure pointer in two or
 more threads.''

 In the case of SQLite, it would be cleaner to ''not'' reuse
 a connection once the thread which created it terminates.
 The database pool idea could be kept, as it always allocate
 a connection to the same thread. However, the connection
 should be destroyed after its last use, instead of being
 put back in the pool. With this change, it is then possible
 to use the thread safe version of the SQLite library:

 {{{
 #!diff
 --- trac/db.py        Thu Sep 29 10:13:36 2005 +0200
 +++ trac/db.py        Mon Oct 10 10:27:49 2005 +0200
 @@ -182,9 +182,7 @@
                  else:
                      del self._active[tid]
                      if cnx not in self._dormant:
 -                        cnx.rollback()
 -                        self._dormant.append(cnx)
 -                        self._available.notify()
 +                        del cnx
          finally:
              self._available.release()
 }}}

 (tested with sqlite-3.2.7 and
 pysqlite-2.0.4+[http://initd.org/tracker/pysqlite/ticket/126 #ps126], on
 Linux).

 Another approach would be to not use the database connection
 pool at all, and attach a new db connection to the request object
 (this approach was already suggested in #1721).
 I'm pretty sure this won't affect the performance, quite
 the contrary, but I haven't tested that yet.

-- 
Ticket URL: <http://projects.edgewall.com/trac/ticket/2196>
The Trac Project <http://trac.edgewall.com/>