Use sqlite3_unlock_notify

RESOLVED FIXED in mozilla2.0b1

Status

()

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: sdwilsh, Assigned: sdwilsh)

Tracking

Trunk
mozilla2.0b1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta1+)

Details

(URL)

Attachments

(1 attachment, 9 obsolete attachments)

(Assignee)

Description

9 years ago
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.
(Assignee)

Comment 1

9 years ago
Note: this was originally bent's patch, but I'm taking it over for the time being.
(Assignee)

Comment 2

9 years ago
And I'm going to have to figure out how to test this code, which should be a blast.
(Assignee)

Comment 3

9 years ago
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).
Attachment #450751 - Attachment is obsolete: true
Attachment #451132 - Flags: superreview?(vladimir)
Attachment #451132 - Flags: review?(bugmail)
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.
(Assignee)

Comment 5

9 years ago
(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.
(Assignee)

Comment 6

9 years ago
Looks like I cannot write a test for prepare, which is sadfaces.
(Assignee)

Comment 7

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

This addresses vlad's sr comments.  Tests in just a sec.
Attachment #451132 - Attachment is obsolete: true
Attachment #451409 - Flags: superreview?(vladimir)
Attachment #451409 - Flags: review?(bugmail)
Attachment #451132 - Flags: superreview?(vladimir)
Attachment #451132 - Flags: review?(bugmail)
(Assignee)

Comment 8

9 years ago
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.
Attachment #451410 - Flags: review?(bugmail)
Attachment #451410 - Flags: review?(bent.mozilla)
(Assignee)

Updated

9 years ago
Whiteboard: [needs review bent][needs review asuth][needs sr vlad]
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?
Attachment #451409 - Flags: superreview?(vladimir) → superreview+
(Assignee)

Comment 10

9 years ago
(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).
(Assignee)

Updated

9 years ago
Whiteboard: [needs review bent][needs review asuth][needs sr vlad] → [needs review bent][needs review 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.
Attachment #451410 - Flags: review?(bugmail) → review-
(Assignee)

Comment 12

9 years ago
(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...
(Assignee)

Updated

9 years ago
Whiteboard: [needs review bent][needs review asuth] → [needs review bent]
(Assignee)

Comment 13

9 years ago
(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).
(Assignee)

Comment 14

9 years ago
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
Attachment #451409 - Attachment is obsolete: true
Attachment #451410 - Attachment is obsolete: true
Attachment #452315 - Flags: review?(bugmail)
Attachment #452315 - Flags: review?(bent.mozilla)
Attachment #451410 - Flags: review?(bent.mozilla)
Attachment #451409 - Flags: review?(bugmail)
(Assignee)

Updated

9 years ago
Whiteboard: [needs review bent] → [needs review bent][needs review asuth]
(Assignee)

Comment 15

9 years ago
This is blocking a beta blocker, so requesting blocking...
blocking2.0: --- → ?
(Assignee)

Comment 16

9 years ago
(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.
(Assignee)

Updated

9 years ago
blocking2.0: ? → beta1+
(Assignee)

Comment 17

9 years ago
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...
Attachment #452315 - Attachment is obsolete: true
Attachment #452315 - Flags: review?(bugmail)
Attachment #452315 - Flags: review?(bent.mozilla)
(Assignee)

Comment 18

9 years ago
Pretty sure I found a bug in SQLite here...talking to drh about it.
(Assignee)

Comment 19

9 years ago
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.
(Assignee)

Comment 20

9 years ago
Created attachment 452795 [details] [diff] [review]
v1.4
Attachment #452365 - Attachment is obsolete: true
Attachment #452795 - Flags: review?(bugmail)
Attachment #452795 - Flags: review?(bent.mozilla)
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?
(Assignee)

Comment 22

9 years ago
(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.
(Assignee)

Comment 23

9 years ago
Created attachment 452835 [details] [diff] [review]
v1.5

per comments
Attachment #452795 - Attachment is obsolete: true
Attachment #452835 - Flags: review?(bugmail)
Attachment #452835 - Flags: review?(bent.mozilla)
Attachment #452795 - Flags: review?(bugmail)
Attachment #452795 - Flags: review?(bent.mozilla)
Comment on attachment 452835 [details] [diff] [review]
v1.5

r=me with the srv filtering when returning from prepareStmt that we discussed on irc.
Attachment #452835 - Flags: review?(bent.mozilla) → review+
(Assignee)

Updated

9 years ago
Whiteboard: [needs review bent][needs review asuth] → [needs review asuth]
(Assignee)

Comment 25

9 years ago
Created attachment 452849 [details] [diff] [review]
v1.6

per review comments
Attachment #452835 - Attachment is obsolete: true
Attachment #452849 - Flags: review?(bugmail)
Attachment #452835 - Flags: review?(bugmail)
(Assignee)

Comment 26

9 years ago
Created attachment 452852 [details] [diff] [review]
v1.7

+ fix a typo in configure.in
Attachment #452849 - Attachment is obsolete: true
Attachment #452852 - Flags: review?(bugmail)
Attachment #452849 - Flags: review?(bugmail)
Attachment #452852 - Flags: review?(bugmail) → review+
(Assignee)

Comment 27

9 years ago
Pushed this to the try server to make sure all is well.
Whiteboard: [needs review asuth]
(Assignee)

Comment 28

9 years ago
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.
(Assignee)

Comment 29

9 years ago
Word.

http://hg.mozilla.org/mozilla-central/rev/bf063aaac85c
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a6
(Assignee)

Updated

9 years ago
Depends on: 583502
You need to log in before you can comment on or make changes to this bug.