Closed
Bug 571599
Opened 13 years ago
Closed 13 years ago
Use sqlite3_unlock_notify
Categories
(Toolkit :: Storage, defect)
Toolkit
Storage
Tracking
()
RESOLVED
FIXED
mozilla2.0b1
Tracking | Status | |
---|---|---|
blocking2.0 | --- | beta1+ |
People
(Reporter: sdwilsh, Assigned: sdwilsh)
References
()
Details
Attachments
(1 file, 9 obsolete files)
22.37 KB,
patch
|
asuth
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
Note: this was originally bent's patch, but I'm taking it over for the time being.
Assignee | ||
Comment 2•13 years ago
|
||
And I'm going to have to figure out how to test this code, which should be a blast.
Assignee | ||
Comment 3•13 years ago
|
||
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•13 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•13 years ago
|
||
Looks like I cannot write a test for prepare, which is sadfaces.
Assignee | ||
Comment 7•13 years ago
|
||
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•13 years ago
|
||
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•13 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•13 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•13 years ago
|
Whiteboard: [needs review bent][needs review asuth][needs sr vlad] → [needs review bent][needs review asuth]
Comment 11•13 years ago
|
||
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•13 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•13 years ago
|
Whiteboard: [needs review bent][needs review asuth] → [needs review bent]
Assignee | ||
Comment 13•13 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•13 years ago
|
||
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•13 years ago
|
Whiteboard: [needs review bent] → [needs review bent][needs review asuth]
Assignee | ||
Comment 15•13 years ago
|
||
This is blocking a beta blocker, so requesting blocking...
blocking2.0: --- → ?
Assignee | ||
Comment 16•13 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•13 years ago
|
blocking2.0: ? → beta1+
Assignee | ||
Comment 17•13 years ago
|
||
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•13 years ago
|
||
Pretty sure I found a bug in SQLite here...talking to drh about it.
Assignee | ||
Comment 19•13 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•13 years ago
|
||
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•13 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•13 years ago
|
||
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•13 years ago
|
Whiteboard: [needs review bent][needs review asuth] → [needs review asuth]
Assignee | ||
Comment 25•13 years ago
|
||
per review comments
Attachment #452835 -
Attachment is obsolete: true
Attachment #452849 -
Flags: review?(bugmail)
Attachment #452835 -
Flags: review?(bugmail)
Assignee | ||
Comment 26•13 years ago
|
||
+ fix a typo in configure.in
Attachment #452849 -
Attachment is obsolete: true
Attachment #452852 -
Flags: review?(bugmail)
Attachment #452849 -
Flags: review?(bugmail)
Updated•13 years ago
|
Attachment #452852 -
Flags: review?(bugmail) → review+
Assignee | ||
Comment 27•13 years ago
|
||
Pushed this to the try server to make sure all is well.
Whiteboard: [needs review asuth]
Assignee | ||
Comment 28•13 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•13 years ago
|
||
Word. http://hg.mozilla.org/mozilla-central/rev/bf063aaac85c
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a6
You need to log in
before you can comment on or make changes to this bug.
Description
•