571599 - Use sqlite3_unlock_notify

Status: RESOLVED FIXED

Description

[Shawn Wilsher :sdwilsh]

Created attachment 450751 [details] [diff] [review]
v0.1

Looks like we need this to do IndexedDB properly.  It is considered to be an experimental API, so care should be taken, but I'm sure we could get that reclassified.

This patch isn't ready for review yet (I need to look it over still), but putting it up here so interested parties can start digging in if they care to.

Comment 1

[Shawn Wilsher :sdwilsh]

Note: this was originally bent's patch, but I'm taking it over for the time being.

Comment 2

[Shawn Wilsher :sdwilsh]

And I'm going to have to figure out how to test this code, which should be a blast.

Comment 3

[Shawn Wilsher :sdwilsh]

Created attachment 451132 [details] [diff] [review]
Code Changes v1.0

Code changes with my comments addressed.  Asking for sr here because this is a largish change that I think might benefit from sr.

Going to work on a patch with some tests next, but wanted to get this up (this is passing IndexedDB tests, and they'd be all sorts of faily if this wasn't working).

Comment 4

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

Comment on attachment 451132 [details] [diff] [review]
Code Changes v1.0

This looks fine to me; I assume the anonymous namespace usage is normal within storage, otherwise it looks a little strange (I also might have put the notification method as a static member method of the struct, eliminating the need for an explicit Signal() method, but it's fine as-is).

From reading the docs though, does this need to handle the DROP TABLE case?  Specifically, in step(), it looks like it needs to query sqlite3_extended_errcode() for SQLITE_LOCKED_SHARED_CACHE, and not just SQLITE_LOCKED as the result.  One way to do it might be to check the extended error code after WaitForUnlockNotify, that way avoiding the extra function call in the normal case.

Comment 5

[Shawn Wilsher :sdwilsh]

(In reply to comment #4)
> From reading the docs though, does this need to handle the DROP TABLE case? 
> Specifically, in step(), it looks like it needs to query
> sqlite3_extended_errcode() for SQLITE_LOCKED_SHARED_CACHE, and not just
> SQLITE_LOCKED as the result.  One way to do it might be to check the extended
> error code after WaitForUnlockNotify, that way avoiding the extra function call
> in the normal case.
Yeah, probably should handle it.  I'll have to add a test for it too.

Comment 6

[Shawn Wilsher :sdwilsh]

Looks like I cannot write a test for prepare, which is sadfaces.

Comment 7

[Shawn Wilsher :sdwilsh]

Created attachment 451409 [details] [diff] [review]
Code Changes v1.1

This addresses vlad's sr comments.  Tests in just a sec.

Comment 8

[Shawn Wilsher :sdwilsh]

Created attachment 451410 [details] [diff] [review]
Tests v1.0

It's been a long while since I played with condition variables, so asking for r from bent too.

Comment 9

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

Comment on attachment 451409 [details] [diff] [review]
Code Changes v1.1

looks fine, but why enable/disable extended result codes?  Can't you just call sqlite3_extended_errcode() if we need to disambiguate?

Comment 10

[Shawn Wilsher :sdwilsh]

(In reply to comment #9)
> (From update of attachment 451409 [details] [diff] [review])
> looks fine, but why enable/disable extended result codes?  Can't you just call
> sqlite3_extended_errcode() if we need to disambiguate?
The problem is that I'd have to hold the database lock long enough to do both calls, which I think is undesirable.  The call is cheap enough that I'm not worried about it (it just changes a mask).

Comment 11

[Andrew Sutherland [:asuth]]

Comment on attachment 451410 [details] [diff] [review]
Tests v1.0

In the code patch, I think convertResultCode should defensively mask off the high bits in case there is leakage of extended result codes in the multiple-threads-using-a-single connection case which would result in the generic NS_ERROR_FAILURE case that likes to NS_WARNING.

In the unit test patch, you use "const char const* mSQL;" twice and gcc gets pissy (error).  I presume you meant "const char * const mSQL" (const binds left, except when it runs out of left).

Your test also fails on line 141, which is reassuring because the DROP tests were confusing to me since they were asserting that the DROPs succeeded when they're supposed to fail.  I also think you need to add more synchronization with DatabaseTester so that you make sure the background threads actually run their checks before teardown happens.  I realize that XPCOM teardown will cause the threads to run to completion and they will print error messages, but that can still happen after you print your cool summary of how many tests passed (which is potentially confusing).

Please correct the tests and push to the try server and paste the revision here.

Comment 12

[Shawn Wilsher :sdwilsh]

(In reply to comment #11)
> (From update of attachment 451410 [details] [diff] [review])
> In the code patch, I think convertResultCode should defensively mask off the
> high bits in case there is leakage of extended result codes in the
> multiple-threads-using-a-single connection case which would result in the
> generic NS_ERROR_FAILURE case that likes to NS_WARNING.
Good idea.

> In the unit test patch, you use "const char const* mSQL;" twice and gcc gets
> pissy (error).  I presume you meant "const char * const mSQL" (const binds
> left, except when it runs out of left).
Uh, yes.  const is hard :(

> Your test also fails on line 141, which is reassuring because the DROP tests
> were confusing to me since they were asserting that the DROPs succeeded when
> they're supposed to fail.  I also think you need to add more synchronization
> with DatabaseTester so that you make sure the background threads actually run
> their checks before teardown happens.  I realize that XPCOM teardown will cause
> the threads to run to completion and they will print error messages, but that
> can still happen after you print your cool summary of how many tests passed
> (which is potentially confusing).
This is odd since I know I've pushed this to the try server...

Comment 13

[Shawn Wilsher :sdwilsh]

(In reply to comment #11)
> Your test also fails on line 141, which is reassuring because the DROP tests
> were confusing to me since they were asserting that the DROPs succeeded when
> they're supposed to fail.  I also think you need to add more synchronization
> with DatabaseTester so that you make sure the background threads actually run
> their checks before teardown happens.  I realize that XPCOM teardown will cause
> the threads to run to completion and they will print error messages, but that
> can still happen after you print your cool summary of how many tests passed
> (which is potentially confusing).
The synchronization issue explains why they were passing for me.  Adding an additionally Wait in the test function now makes them fail (with a Notify at the end of DatabaseTester).

Comment 14

[Shawn Wilsher :sdwilsh]

Created attachment 452315 [details] [diff] [review]
v1.2

Addresses comments, passes tests locally, and rolls both patches together.  Pushed to try here:
http://hg.mozilla.org/try/rev/649cefd96dee

Comment 15

[Shawn Wilsher :sdwilsh]

This is blocking a beta blocker, so requesting blocking...

Comment 16

[Shawn Wilsher :sdwilsh]

(In reply to comment #11)
> In the unit test patch, you use "const char const* mSQL;" twice and gcc gets
> pissy (error).  I presume you meant "const char * const mSQL" (const binds
> left, except when it runs out of left).
Whoops, forgot to do this.  Fixed locally, but now the test isn't passing (weird).  Debugging.

Comment 17

[Shawn Wilsher :sdwilsh]

Created attachment 452365 [details] [diff] [review]
v1.3

Well this is better, but it seems like I have something like a deadlock.  It seems like in the drop cases SQLite is actually returning SQLITE_LOCKED_SHAREDCACHE, which seems contrary to what the docs say.  I'm doing a full rebuild now to make sure I've rebuilt everything, but I'm a bit stumped...

Comment 18

[Shawn Wilsher :sdwilsh]

Pretty sure I found a bug in SQLite here...talking to drh about it.

Comment 19

[Shawn Wilsher :sdwilsh]

Clarified with drh:
If two database connections, then SQLITE_LOCKED_SHAREDCACHE is appropriate because the DROP TABLE can wait for the SELECT to finish.  Only when the DROP TABLE occurs on the same connection as the SELECT should SQLITE_LOCKED be returned.

They'll be updating the documentation, and I'll be updating my tests.

Comment 20

[Shawn Wilsher :sdwilsh]

Created attachment 452795 [details] [diff] [review]
v1.4

Comment 21

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 452795 [details] [diff] [review]
v1.4

>+  if (srv == SQLITE_OK) {
>+    notification.Wait();
>+  }

Looks like prevailing style says no braces here.

>+  (void)::sqlite3_extended_result_codes(db, 1);

Hm. I'm not sure you want to have this set for anything other than the step/prepare call itself. You're leaving it set all the way until we return here. Couldn't sqlite3_unlock_notify return something that has extended bits on it?

>+int step(sqlite3_stmt *aStatement);
>+int prepare(sqlite3 *aDatabase, const nsCString &aSQL, sqlite3_stmt **_stmt);

Shouldn't these have more descriptive names? This is too short and vague IMO.

>+    nsCAutoString sql(::sqlite3_sql(mDBStatement));

I'm 95% sure you want nsDependentCString here.

>+already_AddRefed<mozIStorageConnection>
>+getDatabase()
> ...
>+  (void)dbFile->Append(NS_LITERAL_STRING("storage_test_db.sqlite"));

Why wouldn't you check errors? This is a test after all...

>+class DatabaseLocker : public nsRunnable
> ...
>+class DatabaseTester : public nsRunnable

DatabaseTester should inherit DatabaseLocker and override Run. You're duplicating a bunch.

>+setup()

Again, why wouldn't you check errors here?

Comment 22

[Shawn Wilsher :sdwilsh]

(In reply to comment #21)
> Hm. I'm not sure you want to have this set for anything other than the
> step/prepare call itself. You're leaving it set all the way until we return
> here. Couldn't sqlite3_unlock_notify return something that has extended bits on
> it?
This is why convertResultCode automagically handles it by masking.

> Shouldn't these have more descriptive names? This is too short and vague IMO.
Will rename to *Stmt.

Comment 23

[Shawn Wilsher :sdwilsh]

Created attachment 452835 [details] [diff] [review]
v1.5

per comments

Comment 24

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 452835 [details] [diff] [review]
v1.5

r=me with the srv filtering when returning from prepareStmt that we discussed on irc.

Comment 25

[Shawn Wilsher :sdwilsh]

Created attachment 452849 [details] [diff] [review]
v1.6

per review comments

Comment 26

[Shawn Wilsher :sdwilsh]

Created attachment 452852 [details] [diff] [review]
v1.7

+ fix a typo in configure.in

Comment 27

[Shawn Wilsher :sdwilsh]

Pushed this to the try server to make sure all is well.

Comment 28

[Shawn Wilsher :sdwilsh]

Need to tweak the test a bit.  Getting a spurious failure on the try server runs:
###!!! ASSERTION: Must close the database on the thread that you opened it with!: 'Error', file /builds/slave/tryserver-linux64-debug/build/storage/src/mozStorageConnection.cpp, line 536

Not going to ask for review on it though.

Comment 29

[Shawn Wilsher :sdwilsh]

Word.

http://hg.mozilla.org/mozilla-central/rev/bf063aaac85c