314635 - SQLite not built with THREADSAFE preprocessor macro

Status: RESOLVED FIXED

(Toolkit :: Storage, defect)

Description

[Matthew Gertner]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6

The SQLite library included in the storage module isn't built with the THREADSAFE preprocessor macro (as far as I can see). As explained in the reference URL, the library is threadsafe but only if the macro is defined. Without this, a database cannot be used from multiple threads. This would be very useful (almost essential) for us in order to support parallel transactions in separate threads. I imagine it would be useful for others.

Unless there is a compelling reason why this can't be done, I would suggest defining the macro when building the SQLite library.

Reproducible: Always

Comment 1

[Matthew Gertner]

Attachment: Proposed patch

Adds THREADSAFE preprocessor macro on all platforms

Comment 2

[Matthew Gertner]

I've posted a proposed patch for the SQLite makefile. To reiterate, without this you can't safely use SQLite from multiple threads. With this patch, everything should work fine, but you have to take care not to use the same database handle across threads. If you need to use a database in multiple threads, it should be opened separately in each thread to obtain a different handle. It should be possible to encapsulate this behavior under mozIStorageConnection using the NSPR thread API so that the different database handles are managed automatically. I can propose a patch for this as well if people think this is the right way to go.

Comment 3

[Benjamin Smedberg]

Vlad, Brett, I'm having trouble remembering now what was decided about multi-thread access.

Comment 4

[Brett Wilson]

We'll definitely have multiple connections open, and I don't think we want to restrict ourselves to one thread for them. It looks like we need this patch.

Comment 5

[Darin Fisher]

Almost everything in Gecko (as well as FF and TB) happens on the main thread.  Is there any use case where we need multi-threaded SQLite?

Comment 6

[Brett Wilson]

(In reply to comment #5)
> Almost everything in Gecko (as well as FF and TB) happens on the main thread. 
> Is there any use case where we need multi-threaded SQLite?

I see where you're going with this.

As best as I can tell, THREADSAFE is only used in file I/O operatsions. The longer term plan is to do all the sql file I/O on a background thread so we can commit without stalling the UI. This would mean that we won't need the THREADSAFE macro at all (you'd still be limited to using your connection from the thread that created it.

For now, I think its best if we include this macro, and the future work will (hopefully) make it obsolete.

Comment 7

[Matthew Gertner]

(In reply to comment #6)
The biggest problem with offloading everything onto a single thread is that you can't run parallel transactions. If I want to run two I/O intensive database operations simultaneously and preserve ACID properties, they need to be run in two separate threads. I think the SQLite people want to add support for named transactions, but I don't think there is a timeframe yet for doing so.

So until named transactions are supported, there seems to be a compelling case for using the THREADSAFE define, particularly if the intention is to give extension authors access to the mozIStorage* APIs.

Comment 8

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #7)
> The biggest problem with offloading everything onto a single thread is that you
> can't run parallel transactions. If I want to run two I/O intensive database
> operations simultaneously and preserve ACID properties, they need to be run in
> two separate threads. I think the SQLite people want to add support for named
> transactions, but I don't think there is a timeframe yet for doing so.

Well, you can't currently run multiple transactions on a single database anyway with SQLite -- the second will block on the first as soon as it tries to obtain write access to the database.  However, we're working with the SQLite folks to get a fix for this.

(In reply to comment #5)
> Almost everything in Gecko (as well as FF and TB) happens on the main thread. 
> Is there any use case where we need multi-threaded SQLite?

This is something that we're going to need to decide -- one of the proposed solutions to the caching problem requires us to have all db access happen on one thread only.  This is fine for UI stuff, but what if we decide to move cache to sqlite as well?  (I'd guess the perf loss should be more than made up for once the sqlite cache is in place.)

Comment 9

[Darin Fisher]

> This is fine for UI stuff, but what if we decide to move
> cache to sqlite as well?

See nsDiskCacheDeviceSQL.cpp in the source.  I only need to access SQLite on the main application thread.

Comment 10

[Matthew Gertner]

(In reply to comment #8)
> Well, you can't currently run multiple transactions on a single database anyway
> with SQLite -- the second will block on the first as soon as it tries to obtain
> write access to the database.  However, we're working with the SQLite folks to
> get a fix for this.

Is there any information available about this effort? I poked around but couldn't find anything.

Comment 11

[Matthew Gertner]

(In reply to comment #9)
> See nsDiskCacheDeviceSQL.cpp in the source.  I only need to access SQLite on
> the main application thread.

I think the key point here is that any decision about multithreading is going to affect extension authors as well. Perhaps there is no need to multiple, long-running, atomic SQL operations directly in the Mozilla code, but it seems likely that extension authors will want to do this. 

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

I think we should allow more multithreading in Mozilla, especially if it can be as easily accomplished as here. Most platforms allow multithreading. It seems like we want storage to become a sorta base API for the platform; the other ones mozilla uses are threadsafe already (NSPR, NSS, XPCOM).

And if we want to do things like storing SSL certificates in SQLite, we will definitely need this.

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

fixed by bug 328598?

Comment 14

[Brett Wilson]

Yes, this was fixed by bug 328598. HOWEVER, all that means is that we're "building with the threadsafe macro." There are still no guarantees about thread-safety of the asynchronous writer in storage. It may or may not work right now.

What definitely will NOT work is accessing the same database from multiple threads (even from different connections to the same DB). This will require a new software-based locking mechanism to be implemented in the asynchronous writer to replace the file locking that the writer breaks. On top of that, sqlite does not support persistent caching along with multiple connections on different threads, so even if the locking was implemented, the connections will be much slower.

Moral: don't use the DB from multiple threads now.

*** This bug has been marked as a duplicate of 328598 ***

Comment 15

[Brett Wilson]

Sorry, I wanted to mark it fixed, not a dupe.

Comment 16

[Jesse Ruderman]

I think this change was reversed in bug 330340 because it broke the build on BeOS.

Comment 17

[Brett Wilson]

The previous reverse of this patch is going to be reversed in bug 336314.