SQLite

View Ticket
Login
Ticket Hash: efeeb90f2ac169bbb8e12d59a3576dcec5aaac6c
Title: error messages should be per-thread not global
Status: Closed Type: Feature_Request
Severity: Important Priority: Immediate
Subsystem: Unknown Resolution: Rejected
Last Modified: 2014-03-11 12:59:44
Version Found In: 3.6.5
Description:
See http://www.sqlite.org/cvstrac/tktview?tn=3498

SQLite error messages are currently stored as a single instance on the connection handle.  The error message API returns an integer or char* of the most recent error in any thread.  Consequently if the connection is used in multiple threads concurrently then there is a race condition between an error happening in an API and retrieving the information.  (In the case of the char* it could now point to bogus memory and result in a crash.)

The way this issue is handled in other libraries and operating systems is to have the error information be stored per thread.  For example errno on POSIX and GetLastError() on Windows both behave this way.

The current documented workaround is to call sqlite3_mutex_enter(sqlite3_db_mutex(D)) before each SQLite api call, retrieve the error message/code if applicable and then release the mutex.  This adds overhead (at a guess 1% CPU), and requires every SQLite call to be "wrapped" by the caller.  It also requires the developers to have read the API page and understood the issue.  This issue should also be pointed out at http://www.sqlite.org/threadsafe.html

Making the error code/string be per thread unilaterally will break any existing code that does things like an operation in one thread and retrieval of the error code/message in a second thread.  At a guess this kind of code is very rare.

Examining real world code <a href="http://www.google.com/codesearch?q=sqlite3_errmsg">shows</a> that existing open source projects out there do not get this handling correct.  One example is the Dalvik virtual machine used in Android. (The issue is only a problem if a database connection is used by more than one thread which is harder to determine, but I believe Android does allow it.)

Going forward I would say the current approach of requiring an extra layer of taking a mutex around calls AND error retrieval has not been followed and will result in "random" crashes for programs unless they do.  It also goes against the way error handling is normally done (cf errno and GetLastError()).  Looking at existing code, it would also be tricky to do on many codebases anyway.

I suggest changing SQLite to behave such that thread local errors are used.  This will make existing code work "normally" again, and not require the extra layer of locking (although there is no harm if it is still present).  The codebases that will have problems are those doing API calls and error retrieval in different threads.  That can be handled by a #define making one of the behaviours default, or a per connection flag to say if there is one error stored or one per thread.