Bug 40607 - Implement the sync DB API in workers
Summary: Implement the sync DB API in workers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Dumitru Daniliuc
URL:
Keywords:
Depends on: 41216
Blocks: 34990
  Show dependency treegraph
 
Reported: 2010-06-14 20:01 PDT by Dumitru Daniliuc
Modified: 2010-07-22 13:15 PDT (History)
13 users (show)

See Also:


Attachments
patch #1: implement DatabaseSync::openDatabaseSync() (21.05 KB, patch)
2010-06-14 20:29 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #2: Add the SQLException class (24.07 KB, patch)
2010-06-17 03:05 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #2: Add the SQLException class (28.94 KB, patch)
2010-06-17 14:43 PDT, Dumitru Daniliuc
no flags Details | Formatted Diff | Diff
patch #2: Add the SQLException class (44.77 KB, patch)
2010-06-18 04:30 PDT, Dumitru Daniliuc
abarth: review+
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #1: implement DatabaseSync::openDatabaseSync() (56.33 KB, patch)
2010-06-21 18:04 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #1: implement DatabaseSync::openDatabaseSync() (58.09 KB, patch)
2010-06-23 12:58 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #1: implement DatabaseSync::openDatabaseSync() (64.78 KB, patch)
2010-06-23 15:17 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #1: implement DatabaseSync::openDatabaseSync() (64.46 KB, patch)
2010-06-23 16:14 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #1: implement DatabaseSync::openDatabaseSync() (64.65 KB, patch)
2010-06-24 13:49 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #1: implement DatabaseSync::openDatabaseSync() (64.75 KB, patch)
2010-06-24 17:32 PDT, Dumitru Daniliuc
no flags Details | Formatted Diff | Diff
patch #1: implement DatabaseSync::openDatabaseSync() (71.18 KB, patch)
2010-06-26 04:56 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #1: implement DatabaseSync::openDatabaseSync() (71.10 KB, patch)
2010-06-26 05:19 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #1: implement DatabaseSync::openDatabaseSync() (70.80 KB, patch)
2010-06-29 16:03 PDT, Dumitru Daniliuc
fishd: review+
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests (115.40 KB, patch)
2010-07-01 18:51 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests (115.41 KB, patch)
2010-07-02 15:24 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests (131.69 KB, patch)
2010-07-03 02:54 PDT, Dumitru Daniliuc
abarth: review-
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests (153.71 KB, patch)
2010-07-07 17:34 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests (153.75 KB, patch)
2010-07-07 18:03 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests (149.99 KB, patch)
2010-07-09 01:34 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests (150.01 KB, patch)
2010-07-09 02:56 PDT, Dumitru Daniliuc
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests (149.84 KB, patch)
2010-07-09 18:21 PDT, Dumitru Daniliuc
fishd: review+
dumi: commit-queue-
Details | Formatted Diff | Diff
patch #4: interrupt all DB operations when the worker is shutting down (31.70 KB, patch)
2010-07-22 01:47 PDT, Dumitru Daniliuc
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dumitru Daniliuc 2010-06-14 20:01:29 PDT
Implement the sync DB API in workers. If there's another bug opened for this already, please merge them -- I couldn't find any.
Comment 1 Dumitru Daniliuc 2010-06-14 20:29:46 PDT
Created attachment 58744 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

All new code in DatabaseSync.cpp was copy-pasted from Database.cpp (with a couple of trivial changes, such as removing calls to DatabaseThread).

The plan is to:
1. Implement DatabaseSync.cpp one "feature" at a time (openDatabaseSync(), transaction(), clean up everything properly when the worker shuts down in the middle of a DB operation, etc.)
2. Copy-paste code from Database.cpp to DatabaseSync.cpp and modify it as needed, in order to make each feature in DatabaseSync.cpp work.
3. Once feature 'n' is landed in DatabaseSync.cpp, work on feature 'n + 1', and in parallel work on moving the duplicate code introduced in feature 'n' to AbstractDatabase.

I think the advantages of this approach are:
1. We _know_ what code is duplicated when moving it from Database/DatabaseSync to AbstractDatabase, instead of guessing.
2. Moving duplicate code to AbstractDatabase doesn't block the DatabaseSync implementation; instead, it can be done in parallel.
Comment 2 WebKit Review Bot 2010-06-14 20:30:25 PDT
Attachment 58744 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/storage/DatabaseSync.cpp:343:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Dumitru Daniliuc 2010-06-14 20:34:17 PDT
> WebCore/storage/DatabaseSync.cpp:343:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]

This code is copy-pasted from Database.cpp. I think it might be easier to review this patch if the new code in DatabaseSync matches exactly the existing one in Database. I can fix this style problem when moving this common code to AbstractDatabase.
Comment 4 Dumitru Daniliuc 2010-06-17 03:05:46 PDT
Created attachment 58972 [details]
patch #2: Add the SQLException class
Comment 5 WebKit Review Bot 2010-06-17 03:41:30 PDT
Attachment 58972 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/bindings/js/JSExceptionBase.cpp:39:  One or more unexpected \r (^M) found; better to use only a \n  [whitespace/carriage_return] [1]
Suppressing further [whitespace/carriage_return] reports for this file.
Total errors found: 7 in 20 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 6 WebKit Review Bot 2010-06-17 04:07:51 PDT
Attachment 58972 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3278278
Comment 7 Dumitru Daniliuc 2010-06-17 14:43:51 PDT
Created attachment 59039 [details]
patch #2: Add the SQLException class

Same patch, converted line endings in JSExceptionBase.cpp from CRLF to LF.

The Chromium bot fails because apparently it misses a step that regenerates .h/.cpp files from .idl files. I tried the patch in a local Chromium client and had no issues with it.
Comment 8 WebKit Review Bot 2010-06-17 16:10:06 PDT
Attachment 59039 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3295338
Comment 9 Adam Barth 2010-06-17 16:57:04 PDT
Comment on attachment 59039 [details]
patch #2: Add the SQLException class

WebCore/bindings/js/JSExceptionBase.cpp:65
 +          return reinterpret_cast<ExceptionBase*>(pathException);
Why does this diff look so funny?

WebCore/page/DOMWindow.idl:733
 +          attribute SQLExceptionConstructor SQLException;
Do you need to update the results of the tests that enumerate the global object for all the ports?

WebCore/storage/SQLException.h:7
 +   * version 2 of the License, or (at your option) any later version.
2.0?  I thought we used 2.1..  Google usually contributes under BSD.
Comment 10 Adam Barth 2010-06-17 16:57:33 PDT
Did you run the tests?  That patch is likely to need updated expectations for some.
Comment 11 Dumitru Daniliuc 2010-06-17 17:19:49 PDT
(In reply to comment #9)
> (From update of attachment 59039 [details])
> WebCore/bindings/js/JSExceptionBase.cpp:65
>  +          return reinterpret_cast<ExceptionBase*>(pathException);
> Why does this diff look so funny?

JSExceptionBase.cpp currently uses CRLF line endings, and I had to convert them all to LF to make the style checker happy.

Working on the other comments.
Comment 12 Dumitru Daniliuc 2010-06-18 04:30:10 PDT
Created attachment 59095 [details]
patch #2: Add the SQLException class

patch #2 should be ready for another look.

Fixed the copyright header.

Fixed the expectations of the failing tests on Mac + the expectations for those tests on all other ports that I could find.

The patch passes all tests on Mac; a whole bunch of tests fail in the Qt port, but none of them seems related to this patch (most of them are in css1/, editing/ and fast/dom/Geolocation/ -- maybe I didn't pass in some flags?).
Comment 13 WebKit Review Bot 2010-06-18 05:08:12 PDT
Attachment 59095 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3266343
Comment 14 Adam Barth 2010-06-19 17:19:13 PDT
Comment on attachment 59095 [details]
patch #2: Add the SQLException class

Adding things to the global scope is a bit of a pain.  Please watch the bots carefully to make sure you've got all the expectation updates.

WebCore/WebCore.xcodeproj/project.pbxproj:11037
 +  				81CC114011BEAA9D00D0D856 /* IDBKeyRange.idl */,
Did you mean to add these files?
Comment 15 Dumitru Daniliuc 2010-06-20 17:27:16 PDT
> WebCore/WebCore.xcodeproj/project.pbxproj:11037
>  +                  81CC114011BEAA9D00D0D856 /* IDBKeyRange.idl */,
> Did you mean to add these files?

No. Hmm, not sure how they got there. Thanks for catching this!
Comment 16 Dumitru Daniliuc 2010-06-21 00:10:50 PDT
patch #2 landed as r61531.
Comment 17 WebKit Review Bot 2010-06-21 01:03:31 PDT
http://trac.webkit.org/changeset/61531 might have broken GTK Linux 64-bit Debug
Comment 18 Dumitru Daniliuc 2010-06-21 01:04:47 PDT
(In reply to comment #17)
> http://trac.webkit.org/changeset/61531 might have broken GTK Linux 64-bit Debug

Missed the GTK expectations for 2 tests, should be fixed now.
Comment 19 Adam Roben (:aroben) 2010-06-21 13:10:10 PDT
Comment on attachment 59095 [details]
patch #2: Add the SQLException class

This change caused fast/dom/prototype-inheritance.html to fail on Windows. New results need to be landed. See http://build.webkit.org/results/Windows%20Debug%20(Tests)/r61558%20(15282)/results.html
Comment 20 Dumitru Daniliuc 2010-06-21 13:27:46 PDT
(In reply to comment #19)
> (From update of attachment 59095 [details])
> This change caused fast/dom/prototype-inheritance.html to fail on Windows. New results need to be landed. See http://build.webkit.org/results/Windows%20Debug%20(Tests)/r61558%20(15282)/results.html

Updated the expectations in r61570.
Comment 21 Dumitru Daniliuc 2010-06-21 18:04:53 PDT
Created attachment 59320 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

Michael, I changed patch #1 as we discussed (I think...). Basically, I moved performOpenAndVerify() + some fields + simple getters for those fields from Database to AbstractDatabase. I also moved all guid-related methods to AbstractDatabase, but the plan is to move them to a separate class once this patch is landed. I also made a trivial change to DatabaseTracker, in order to remove the dependency on Database. I haven't changed the logic at all.

I can already see a few ways to clean up Database further, but I'll work on those in separate patches.

All tests in storage/ passed.
Comment 22 WebKit Review Bot 2010-06-21 18:07:18 PDT
Attachment 59320 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/storage/AbstractDatabase.cpp:334:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
WebCore/storage/DatabaseAuthorizer.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
WebCore/storage/Database.cpp:477:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 3 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 Dumitru Daniliuc 2010-06-21 18:21:55 PDT
> Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
> WebCore/storage/AbstractDatabase.cpp:334:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
> WebCore/storage/DatabaseAuthorizer.h:34:  Alphabetical sorting problem.  [build/include_order] [4]
> WebCore/storage/Database.cpp:477:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Total errors found: 3 in 12 files

Fix all style problems.
Comment 24 Michael Nordman 2010-06-22 12:47:21 PDT
> void DatabaseSync::markAsDeletedAndClose()
> {
>     // FIXME: we probably need to do something else in here other than calling closeImmediately().
>     closeImmediately();
> }
> 
> void DatabaseSync::closeImmediately()
> {
>     // FIXME: probably need to do more things in here
>     closeDatabase();
>     DatabaseTracker::tracker().removeOpenDatabase(this);
> }

These methods are not called on the ScriptExecutionContext's thread, but the closeDatabase() method uses the sqlite3 database handle. That looks like a threading violation that needs to be resolved.
Comment 25 Eric Seidel (no email) 2010-06-22 13:24:33 PDT
Comment on attachment 59039 [details]
patch #2: Add the SQLException class

Cleared Adam Barth's review+ from obsolete attachment 59039 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 26 Dumitru Daniliuc 2010-06-23 12:58:57 PDT
Created attachment 59556 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

(In reply to comment #24)
> > void DatabaseSync::markAsDeletedAndClose()
> > {
> >     // FIXME: we probably need to do something else in here other than calling closeImmediately().
> >     closeImmediately();
> > }
> > 
> > void DatabaseSync::closeImmediately()
> > {
> >     // FIXME: probably need to do more things in here
> >     closeDatabase();
> >     DatabaseTracker::tracker().removeOpenDatabase(this);
> > }
> 
> These methods are not called on the ScriptExecutionContext's thread, but the closeDatabase() method uses the sqlite3 database handle. That looks like a threading violation that needs to be resolved.

fixed.
Comment 27 Dumitru Daniliuc 2010-06-23 15:17:23 PDT
Created attachment 59566 [details]
patch #1: implement DatabaseSync::openDatabaseSync()
Comment 28 Dumitru Daniliuc 2010-06-23 16:14:31 PDT
Created attachment 59573 [details]
patch #1: implement DatabaseSync::openDatabaseSync()
Comment 29 Michael Nordman 2010-06-24 11:02:48 PDT
This refactoring LGTM. Mostly moving code, only altering what needs altering to be shared with the sync case.

> Implementing DatabaseSync::openDatabaseSync().

The CL description could use some word smithing to briefly describe the changes made in support of implementing openDatabaseSync().

* Hoisted many methods involved in opening a web database from Database to AbstractDatabase for reuse in DatabaseSync.
* Virtualized performOpenAndVerify() to keep interactions with the DatabaseThread out of the base class as that only applies to the Database class and not DatabaseSync.
* Avoided storing a reference to the creation callback in the base class since its not needed beyond return from the static constructor method in the sync case.
Comment 30 Michael Nordman 2010-06-24 13:26:34 PDT
Oooops... I spoke a little too some on the code part being ready. Couple of things to look at.

* In the usual case where a DatabaseSync is not closed forcibly thru closeImmediately() or markAsDeletedAndClose(), looks like we're leaving dangling pointers in the 'tracker' and cruft in the various GUID map goo maintained by the base class.

* The FIXME comments on closeImmediately() and markAsDeletedAndClose() are vague. Please be more clear about what has to happen or not.

I think you've got a good impl of closeImmediately() in place already and can simply remove the FIXME in that method.

I haven't understood the semantics of markAsDeletedAndClose() until about an hour ago. Similar semantics to closeImmediately() except its expected to be done in a sync fashion and there are additional side effects around the version() method (and around the callers of the public deleted() method). The impl in the most recent patch is not correct since it doesn't behave syncly. I think its OK to leave this undone for now, and just put in a FIXME for it.
Comment 31 Dumitru Daniliuc 2010-06-24 13:49:30 PDT
Created attachment 59696 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

Addressed Michael's comments.
Comment 32 Michael Nordman 2010-06-24 17:07:27 PDT
Comment on attachment 59696 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

> +void DatabaseSync::closeImmediately()
>  {
> -    ASSERT(m_scriptExecutionContext->isContextThread());
> -    return m_scriptExecutionContext.get();
> +    if (!opened())
> +        return;

I think you should move the call to opened() down so that the method is called on the context thread.

> +
> +    if (!m_scriptExecutionContext->isContextThread()) {
> +        m_scriptExecutionContext->postTask(CloseSyncDatabaseOnContextThreadTask::create(this));
> +        return;
> +    }
> +
> +    DatabaseTracker::tracker().removeOpenDatabase(this);
> +
> +    closeDatabase();
>  }


> DatabaseSync::~DatabaseSync()
> {
>     ASSERT(m_scriptExecutionContext->isContextThread());
> +    closeImmediately();
> }

Oh, thats a virtual method. Probably want to avoid calling that from the dtor.
http://www.artima.com/cppsource/nevercall.html

A seperate, private, non-virtual DatabaseSync::closeAndRemoveFromTracker() method thats only be called on the context thread may help that can be called from the virtual closeImmediately and from the dtor.
Comment 33 Dumitru Daniliuc 2010-06-24 17:32:12 PDT
Created attachment 59713 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

(In reply to comment #32)
> (From update of attachment 59696 [details])
> > +void DatabaseSync::closeImmediately()
> >  {
> > -    ASSERT(m_scriptExecutionContext->isContextThread());
> > -    return m_scriptExecutionContext.get();
> > +    if (!opened())
> > +        return;
> 
> I think you should move the call to opened() down so that the method is called on the context thread.

done.

> > DatabaseSync::~DatabaseSync()
> > {
> >     ASSERT(m_scriptExecutionContext->isContextThread());
> > +    closeImmediately();
> > }
> 
> Oh, thats a virtual method. Probably want to avoid calling that from the dtor.
> http://www.artima.com/cppsource/nevercall.html

gotta love C++... i changed the call to closeImmediately() to:

if (opened()) {
    DatabaseTracker.tracker().removeOpenDatabase(this);
    closeDatabase();
}
Comment 34 Michael Nordman 2010-06-24 17:36:17 PDT
lgtm!
Comment 35 Adam Barth 2010-06-24 17:45:01 PDT
Comment on attachment 59713 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

This a pretty big patch to make sure all the code is getting move around properly, but the patch appears correct.

WebCore/storage/AbstractDatabase.cpp:137
 +      if (origin.endsWith("/"))
I still don't understand how origin can ever end with a /

WebCore/storage/AbstractDatabase.cpp:390
 +      DEFINE_STATIC_LOCAL(String, setVersionQuery, ("INSERT INTO " + databaseInfoTableName() + " (key, value) VALUES ('" + databaseVersionKey() + "', ?);"));
Does this mean that this table name is visible to web content?  That would be unfortunate.
Comment 36 Dumitru Daniliuc 2010-06-24 17:53:44 PDT
(In reply to comment #35)
> (From update of attachment 59713 [details])
> This a pretty big patch to make sure all the code is getting move around properly, but the patch appears correct.
> 
> WebCore/storage/AbstractDatabase.cpp:137
>  +      if (origin.endsWith("/"))
> I still don't understand how origin can ever end with a /

Techinically, SecurityOrigin::toString() can return "file://". However, I don't think it matters: we just need a string that uniquely identifies the origin + DB name, and adding another '/' in case of "file://" doesn't change that. So I removed the check and changed this code to always use origin + '/' + name as a unique DB identifier.

> WebCore/storage/AbstractDatabase.cpp:390
>  +      DEFINE_STATIC_LOCAL(String, setVersionQuery, ("INSERT INTO " + databaseInfoTableName() + " (key, value) VALUES ('" + databaseVersionKey() + "', ?);"));
> Does this mean that this table name is visible to web content?  That would be unfortunate.

It's not. DatabaseAuthorizer doesn't allow the web app to do anything on this table (not even read it).
Comment 37 Dumitru Daniliuc 2010-06-24 18:08:02 PDT
patch #1 landed as r61812.
Comment 38 Dumitru Daniliuc 2010-06-26 04:56:21 PDT
Created attachment 59831 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

Same patch, with minor changes:
1. Made ScriptExecutionContext aware of sync DBs, because we need a place to close them before the destructor runs (calling DatabaseTracker::removeOpenDatabase() in the destructor leads to weird things in Chromium).
2. Because of #1, moved 2 sync DB-specific lines from the loop in ScriptExecutionContext::stopDatabases() to Database::stop().
3. Commented out some test cases in the layout test, because they fail in V8. The failure happens because unlike JSC, v8 suppresses the errors in toString(). I'm investigating why this is so, and what can be done to make JSC and V8 behave the same way.
Comment 39 WebKit Review Bot 2010-06-26 04:57:50 PDT
Attachment 59831 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
WebCore/ChangeLog:15:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 40 Dumitru Daniliuc 2010-06-26 05:00:25 PDT
> WebCore/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
> WebCore/ChangeLog:15:  Line contains tab character.  [whitespace/tab] [5]
> Total errors found: 2 in 16 files

Will fix these on landing or when uploading a new version of the patch.
Comment 41 Dumitru Daniliuc 2010-06-26 05:19:18 PDT
Created attachment 59833 [details]
patch #1: implement DatabaseSync::openDatabaseSync()
Comment 42 Eric Seidel (no email) 2010-06-29 03:17:24 PDT
Comment on attachment 59713 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

Cleared Adam Barth's review+ from obsolete attachment 59713 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 43 Michael Nordman 2010-06-29 11:18:56 PDT
Comment on attachment 59833 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

I see where ScriptExecutionContext's collection of open databases has been modified to accept DatabaseSync instances as well as Database instances, but I don't see where the DatabaseSync instances are ever added/removed to/from that collection. Maybe i'm missing something, could you walk me thru how that happens?
Comment 44 Dumitru Daniliuc 2010-06-29 16:03:10 PDT
Created attachment 60066 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

Should be the final version...
Comment 45 Michael Nordman 2010-06-29 16:51:25 PDT
Comment on attachment 60066 [details]
patch #1: implement DatabaseSync::openDatabaseSync()

lgtm (the ChangeLog description should be modified since 4 and 5 no longer apply)

> +        Reviewed by NOBODY (OOPS!).
> +
> +        Implementing DatabaseSync::openDatabaseSync().
> +        https://bugs.webkit.org/show_bug.cgi?id=40607
> +
> +        1. Moved some common code from Database to AbstractDatabase.
> +        2. Made performOpenAndVerify() virtual, since DatabaseSync doesn't
> +           need to interact with DatabaseThread.
> +        3. Removed the m_creationCallback field, since it's only needed in
> +           the openDatabase{Sync} methods.
> +        4. Made ScriptExecutionContext aware of sync DBs, so we can close
> +           the sync DBs before the destructor is called.
> +        5. Moved a couple of async DB-specific lines from
> +           ScriptExecutionContext::stopDatabases() to Database::stop().
Comment 46 Dumitru Daniliuc 2010-06-29 16:58:33 PDT
> lgtm (the ChangeLog description should be modified since 4 and 5 no longer apply)
> 
> > +        Reviewed by NOBODY (OOPS!).
> > +
> > +        Implementing DatabaseSync::openDatabaseSync().
> > +        https://bugs.webkit.org/show_bug.cgi?id=40607
> > +
> > +        1. Moved some common code from Database to AbstractDatabase.
> > +        2. Made performOpenAndVerify() virtual, since DatabaseSync doesn't
> > +           need to interact with DatabaseThread.
> > +        3. Removed the m_creationCallback field, since it's only needed in
> > +           the openDatabase{Sync} methods.
> > +        4. Made ScriptExecutionContext aware of sync DBs, so we can close
> > +           the sync DBs before the destructor is called.
> > +        5. Moved a couple of async DB-specific lines from
> > +           ScriptExecutionContext::stopDatabases() to Database::stop().

oops, will do on landing.
Comment 47 Dumitru Daniliuc 2010-06-29 17:31:11 PDT
patch #1 re-landed as r62153.
Comment 48 Dumitru Daniliuc 2010-07-01 18:51:03 PDT
Created attachment 60325 [details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests
Comment 49 WebKit Review Bot 2010-07-01 18:57:16 PDT
Attachment 60325 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/storage/chromium/SQLTransactionSyncClientChromium.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 65 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 50 Dumitru Daniliuc 2010-07-01 18:58:28 PDT
> WebCore/storage/chromium/SQLTransactionSyncClientChromium.cpp:35:  Alphabetical sorting problem.  [build/include_order] [4]

Fixed. Will wait for more comments before I upload a new patch.
Comment 51 WebKit Review Bot 2010-07-02 03:39:45 PDT
Attachment 60325 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3349346
Comment 52 Dumitru Daniliuc 2010-07-02 15:24:03 PDT
Created attachment 60412 [details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests

Same patch, should fix the style problem, and should build on Chromium.
Comment 53 WebKit Review Bot 2010-07-02 17:01:24 PDT
Attachment 60412 [details] did not build on chromium:
Build output: http://webkit-commit-queue.appspot.com/results/3408082
Comment 54 Dumitru Daniliuc 2010-07-03 02:54:43 PDT
Created attachment 60445 [details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests

Addressed the problems Michael and I talked about offline:
1. An attempt to run a transaction inside another transaction should immediately result in an exception being thrown; added a test for that too.
2. Removed the SQLTransactionSyncClient class, and changed SQLTransactionClient to be usable by both sync and async transactions.
3. Changed SQLStatementSync.cpp to be a 'svn copy' of SQLStatement.cpp.
4. Replaced 'typedef int ExceptionCode' with '#include "ExceptionCode.h"' in all DB header files.
5. Changed the name of the SQLTransactionClient::didCommitTransaction() method to didCommitWriteTransaction().

I also changed '#include <wtf/Threading.h>' to '#include <wtf/ThreadSafeShared.h>' in a few callbacks, removed a few unnecessary #includes in SQLTransactionClientChromium.cpp, and moved the code that runs a transaction to a separate DatabaseSync::runTransaction() method, so it can be shared by DatabaseSync::transaction() and DatabaseSync::changeVersion().
Comment 55 WebKit Review Bot 2010-07-03 02:59:28 PDT
Attachment 60445 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/storage/SQLTransactionSync.cpp:39:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 77 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 56 Dumitru Daniliuc 2010-07-03 04:00:46 PDT
> Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
> WebCore/storage/SQLTransactionSync.cpp:39:  Alphabetical sorting problem.  [build/include_order] [4]
> Total errors found: 1 in 77 files

Fixed, will wait for more comments before re-uploading the patch.
Comment 57 Adam Barth 2010-07-07 04:10:31 PDT
Comment on attachment 60445 [details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests

This patch is huge.  I only reviewed the first half of it and there's already some concerning stuff.  :(

WebCore/storage/DatabaseSync.cpp:95
 +          return new ChangeVersionPreflightStep(expectedVersion);
AdoptRef ???  This looks like a leak.

WebCore/storage/DatabaseSync.cpp:115
 +      ChangeVersionPreflightStep(const String& expectedVersion) : m_expectedVersion(expectedVersion) { }
explicit?  Please make this more than one line.

WebCore/storage/DatabaseSync.cpp:123
 +          return new ChangeVersionPostflightStep(newVersion);
adoptRef.  If you haven't already, please read http://webkit.org/coding/RefPtr.html

WebCore/storage/DatabaseSync.cpp:138
 +      ChangeVersionPostflightStep(const String& newVersion) : m_newVersion(newVersion) { }
explicit, multiline.

WebCore/storage/DatabaseSync.cpp:162
 +      if (!transaction->begin(ec) || !transaction->execute(ec) || !transaction->commit(ec))
Can these return ec and also true?  If not, that seems redundant.

WebCore/storage/SQLStatementSync.cpp:27
 +   */
missing space

WebCore/storage/SQLStatementSync.cpp:59
 +  bool SQLStatement::execute(Database* db)
This function is too complicated for me to understand at 4am.

WebCore/storage/SQLStatementSync.cpp:2
 +   * Copyright (C) 2010 Google Inc. All rights reserved.
Woah there.  You can't just change the copyright on a file from Apple to Google.  What's going on here with the license block?

WebCore/storage/SQLStatementSync.cpp:75
 +      for (unsigned int i = 0; i < m_arguments.size(); ++i) {
We usually just say "unsigned"

WebCore/storage/SQLStatementSync.h:54
 +  
Extra blank line.

WebCore/storage/SQLTransaction.h: 
 +  typedef int ExceptionCode;
Who review this line of code?  It's nonsense.  (Hope it wasn't me!)

WebCore/storage/SQLTransactionClient.h:44
 +  class SQLTransactionClient : public Noncopyable {
If this is a client, why aren't the methods virtual?

WebCore/storage/SQLTransactionSync.cpp:80
 +  PassRefPtr<SQLResultSet> SQLTransactionSync::executeSQL(const String& sqlStatement, const Vector<SQLValue>& arguments, ExceptionCode& ec)
again, too big for my little brain.
Comment 58 Dumitru Daniliuc 2010-07-07 17:34:59 PDT
Created attachment 60812 [details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests

> This patch is huge.

the layout tests are probably more than half of it. another good chunk of it is changes to build files.

> I only reviewed the first half of it and there's already some concerning stuff.  :(

happy to fix them. :)

> WebCore/storage/DatabaseSync.cpp:95
>  +          return new ChangeVersionPreflightStep(expectedVersion);
> AdoptRef ???  This looks like a leak.

fixed.

> WebCore/storage/DatabaseSync.cpp:115
>  +      ChangeVersionPreflightStep(const String& expectedVersion) : m_expectedVersion(expectedVersion) { }
> explicit?  Please make this more than one line.

done.

> WebCore/storage/DatabaseSync.cpp:123
>  +          return new ChangeVersionPostflightStep(newVersion);
> adoptRef.  If you haven't already, please read http://webkit.org/coding/RefPtr.html

done. i thought PassRefPtr::PassRefPtr(T*) and adoptRef() did the same thing.

> WebCore/storage/DatabaseSync.cpp:138
>  +      ChangeVersionPostflightStep(const String& newVersion) : m_newVersion(newVersion) { }
> explicit, multiline.

done.

> WebCore/storage/DatabaseSync.cpp:162
>  +      if (!transaction->begin(ec) || !transaction->execute(ec) || !transaction->commit(ec))
> Can these return ec and also true?  If not, that seems redundant.

you're right, they are redundant. fixed, made these functions return an ExceptionCode.

> WebCore/storage/SQLStatementSync.cpp:27
>  +   */
> missing space

not sure what you mean, didn't change anything.

> WebCore/storage/SQLStatementSync.cpp:59
>  +  bool SQLStatement::execute(Database* db)
> This function is too complicated for me to understand at 4am.

SQLStatement{Sync}::execute() do the following:
1. changes the authorizer mode to read-only, if it's a read-only transaction (to make sure write-statements are disallowed).
2. prepares the statement.
3. makes sure the number of arguments matches the number of '?'s in the statements.
4. bind all arguments.
5. step through the statement, and add data to the result set, one row at a time.

> WebCore/storage/SQLStatementSync.cpp:2
>  +   * Copyright (C) 2010 Google Inc. All rights reserved.
> Woah there.  You can't just change the copyright on a file from Apple to Google.  What's going on here with the license block?

oops, reverted, and added a 1-line google copyright. also fixed the copyright notice in all other sync classes (they should all have the google copyright header).

> WebCore/storage/SQLStatementSync.cpp:75
>  +      for (unsigned int i = 0; i < m_arguments.size(); ++i) {
> We usually just say "unsigned"

fixed.

> WebCore/storage/SQLStatementSync.h:54
>  +  
> Extra blank line.

removed.

> WebCore/storage/SQLTransaction.h: 
>  +  typedef int ExceptionCode;
> Who review this line of code?  It's nonsense.  (Hope it wasn't me!)

it was around since... forever. michael had the same comment, so i removed the 'typedef int ExceptionCode;' line from all DB header files and added '#include "ExceptionCode.h"'.

> WebCore/storage/SQLTransactionClient.h:44
>  +  class SQLTransactionClient : public Noncopyable {
> If this is a client, why aren't the methods virtual?

made them virtual, even though this is the only implementation we have (well, 1 implementation for chromium, and 1 for all other ports), and i think it's quite unlikely we'll ever have another one.

> WebCore/storage/SQLTransactionSync.cpp:80
>  +  PassRefPtr<SQLResultSet> SQLTransactionSync::executeSQL(const String& sqlStatement, const Vector<SQLValue>& arguments, ExceptionCode& ec)
> again, too big for my little brain.

same code as SQLTransaction::executeSQL(), only instead of storing a SQLError, we set a SQLException.
Comment 59 Darin Adler 2010-07-07 17:38:18 PDT
"typedef int ExceptionCode" was historically the standard way to “forward-declare” the ExceptionCode type, so we didn’t have to include ExceptionCode.h in every single header.

The idea was that header contained the actual exception code constants, but if you only needed the WebCore::ExceptionCode type you could typedef it to int. Any files that included both your header and ExceptionCode.h would end up checking it for you. If the typedef didn't match the compiler would generate an error.

Still-older code simply used int, which I felt was less self-documenting.

It’s fine to just include ExceptionCode.h in more places and get rid of that old usage, but it was standard at one time.
Comment 60 Eric Seidel (no email) 2010-07-07 17:58:39 PDT
Attachment 60812 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3448001
Comment 61 Dumitru Daniliuc 2010-07-07 18:03:18 PDT
Created attachment 60817 [details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests

Same patch, should build on Mac.
Comment 62 Adam Barth 2010-07-08 09:01:10 PDT
> It’s fine to just include ExceptionCode.h in more places and get rid of that old usage, but it was standard at one time.

Thanks, I didn't know that.  It seems suboptimal to duplicate that declaration everywhere.  I'm glad we're moving away from that pattern.
Comment 63 Michael Nordman 2010-07-08 18:55:32 PDT
Comment on attachment 60817 [details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests

This is a big patch, I haven't looked at the test cases yet, but I am glad that you have put together a bunch of them to verify that this code works.

WebCore/storage/DatabaseSync.cpp:159
 +      RefPtr<SQLTransactionSync> transaction = transactionPrP;
Do you need to local variable here? Seems like deref'ing the prp is all you need.

WebCore/storage/SQLStatementSync.cpp:122
 +      return resultSet.release();
Can you just return resultSet here the local var is going out of scope momentarily. I'm not really sure which is preferred usage in webkit, please check.

WebCore/storage/SQLTransactionClient.h:49
 +      virtual bool didExceedQuota(AbstractDatabase*);
These really don't have to be virtual. It's not the type of 'client' interface that's is designed to be overriden. Perhaps using the term 'client' was a poor choice in webkit? If the need ever arises we can make them virtual at that time..

WebCore/storage/SQLTransactionSync.cpp:142
 +          ExceptionCode preflightExceptionCode = m_preflightStep->performStep(this);
This pre/post flight mechanism is used to read/write the version stored in the database. Shouldn't we be doing these reads and writes in the transaction?

Are there any other use cases for the pre/post stuff besides the version change use case? It may be overkill for just this use case. Instead of using runTransaction(), the changeVersion() method could be a slightly modified form of runTransaction() essentially.

void changeVersion(...) {
  trans.begin();
  read version from db and compare with expected;
  trans.execute(); // invoke the user callback 
  write new version to db;
  trans.commit();
}

wdyt?

WebCore/storage/SQLTransactionSync.cpp:208
 +      return postflightExceptionCode;
Ditto shouldn't we be doing this in the transaction.
Comment 64 David Levin 2010-07-08 19:23:30 PDT
(In reply to comment #63) 
> WebCore/storage/SQLStatementSync.cpp:122
>  +      return resultSet.release();
> Can you just return resultSet here the local var is going out of scope momentarily. I'm not really sure which is preferred usage in webkit, please check.

fwiw, the way it is written is the preferred usage.

"If a function’s result is a new object or ownership is being transferred for any other reason, the result should be a PassRefPtr. Since local variables are typically RefPtr, it’s common to call release in the return statement to transfer the RefPtr to the PassRefPtr." -- http://webkit.org/coding/RefPtr.html
Comment 65 Darin Adler 2010-07-08 23:33:13 PDT
(In reply to comment #64)
> (In reply to comment #63) 
> > WebCore/storage/SQLStatementSync.cpp:122
> >  +      return resultSet.release();
> > Can you just return resultSet here the local var is going out of scope momentarily. I'm not really sure which is preferred usage in webkit, please check.
> 
> fwiw, the way it is written is the preferred usage.
> 
> "If a function’s result is a new object or ownership is being transferred for any other reason, the result should be a PassRefPtr. Since local variables are typically RefPtr, it’s common to call release in the return statement to transfer the RefPtr to the PassRefPtr." -- http://webkit.org/coding/RefPtr.html

Yes, the release() is preferred.

If you forget to do the release(), then the reference count gets incremented and then decremented. If you remember to do the release(), then the reference count isn't touched. So it's a bit more efficient if you do the release(). There’s no correctness problem either way.
Comment 66 Dumitru Daniliuc 2010-07-09 00:14:28 PDT
> WebCore/storage/DatabaseSync.cpp:159
>  +      RefPtr<SQLTransactionSync> transaction = transactionPrP;
> Do you need to local variable here? Seems like deref'ing the prp is all you need.

removed the runTransaction() method, so this statement went away.

> WebCore/storage/SQLStatementSync.cpp:122
>  +      return resultSet.release();
> Can you just return resultSet here the local var is going out of scope momentarily. I'm not really sure which is preferred usage in webkit, please check.

as Dave and Darin have said, this should be the right way to do this.

> WebCore/storage/SQLTransactionClient.h:49
>  +      virtual bool didExceedQuota(AbstractDatabase*);
> These really don't have to be virtual. It's not the type of 'client' interface that's is designed to be overriden. Perhaps using the term 'client' was a poor choice in webkit? If the need ever arises we can make them virtual at that time..

reverted. i'll see if adam has any objection to this.

> WebCore/storage/SQLTransactionSync.cpp:142
>  +          ExceptionCode preflightExceptionCode = m_preflightStep->performStep(this);
> This pre/post flight mechanism is used to read/write the version stored in the database. Shouldn't we be doing these reads and writes in the transaction?

i think the spec contradicts itself: http://dev.w3.org/html5/webdatabase/#dom-database-sync-changeversion. on one hand, it says that check version + invoke callback + update version should be done atomically. on the other hand, if you look at the step by step description, it looks to me like things should be done in this order:

1. begin transaction
2. check version
3. invoke callback
4. commit transaction
5. update version

i think doing it atomically is the right way to do it, so in the code i moved the "check version" and "update version" steps inside the transaction. i'll send an email to hixie and ask him for a clarification (and we should probably make it more clear in the spec too).

> Are there any other use cases for the pre/post stuff besides the version change use case? It may be overkill for just this use case. Instead of using runTransaction(), the changeVersion() method could be a slightly modified form of runTransaction() essentially.
> 
> void changeVersion(...) {
>   trans.begin();
>   read version from db and compare with expected;
>   trans.execute(); // invoke the user callback 
>   write new version to db;
>   trans.commit();
> }
> 
> wdyt?

done. initially i thought that "optional transaction steps" might be cool, but now i'm thinking that it's quite unlikely that we'll ever need to use them for anything else.

> WebCore/storage/SQLTransactionSync.cpp:208
>  +      return postflightExceptionCode;
> Ditto shouldn't we be doing this in the transaction.

done.
Comment 67 Dumitru Daniliuc 2010-07-09 01:34:58 PDT
Created attachment 61015 [details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests
Comment 68 Eric Seidel (no email) 2010-07-09 01:43:29 PDT
Attachment 61015 [details] did not build on mac:
Build output: http://webkit-commit-queue.appspot.com/results/3499062
Comment 69 Dumitru Daniliuc 2010-07-09 02:56:44 PDT
Created attachment 61025 [details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests

Same patch, should make gcc happy this time.
Comment 70 Michael Nordman 2010-07-09 17:25:22 PDT
Comment on attachment 61025 [details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests

I think the db logic looks correct and the new classes are nicely composed. How the SQLTransactionClient class is used is kind of funky. A stateless object but we're making global static instance.

51 static SQLTransactionClient* transactionClient()
52 {
53     DEFINE_STATIC_LOCAL(SQLTransactionClient, transactionClient, ());
54     return &transactionClient;
55 }

I don't think DEFINE_STATIC_LOCAL construction macro is thread safe and this getter will definitely be called on different threads. It may be better to just add an SQLTransactionClient data member to SQLTransactionSync and call the methods thru that instance.
Comment 71 Michael Nordman 2010-07-09 17:26:29 PDT
otherwise, i think this looks good
Comment 72 Dumitru Daniliuc 2010-07-09 18:21:50 PDT
Created attachment 61134 [details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests

(In reply to comment #70)
> (From update of attachment 61025 [details])
> I think the db logic looks correct and the new classes are nicely composed. How the SQLTransactionClient class is used is kind of funky. A stateless object but we're making global static instance.
> 
> 51 static SQLTransactionClient* transactionClient()
> 52 {
> 53     DEFINE_STATIC_LOCAL(SQLTransactionClient, transactionClient, ());
> 54     return &transactionClient;
> 55 }
> 
> I don't think DEFINE_STATIC_LOCAL construction macro is thread safe and this getter will definitely be called on different threads. It may be better to just add an SQLTransactionClient data member to SQLTransactionSync and call the methods thru that instance.

done.
Comment 73 Michael Nordman 2010-07-13 13:41:59 PDT
Hi webkit reviewers that happen to be chromium developers too. This patch makes it possible to use sync db api in workers. Would be nice to get it into M6.
Comment 74 Darin Fisher (:fishd, Google) 2010-07-13 17:03:53 PDT
Comment on attachment 61134 [details]
patch #3: implementing DatabaseSync::transaction() and changeVersion() + tests

R=me
Comment 75 Dumitru Daniliuc 2010-07-13 21:55:10 PDT
patch #3 landed: r63278.
Comment 76 Michael Nordman 2010-07-14 11:21:52 PDT
Ooops, it looks like the change actually committed is still setting the version outside of the transaction?


93	void DatabaseSync::changeVersion(const String& oldVersion, const String& newVersion, 
...
125	    if ((ec = transaction->commit()))
126	        return;
127	
128	    setExpectedVersion(newVersion);
129	}


> > Are there any other use cases for the pre/post stuff besides the version change use case? It may be overkill
> > for just this use case. Instead of using runTransaction(), the changeVersion() method could be a slightly 
> > modified form of runTransaction() essentially.
> > 
> > void changeVersion(...) {
> >   trans.begin();
> >   read version from db and compare with expected;
> >   trans.execute(); // invoke the user callback 
> >   write new version to db;
> >   trans.commit();
> > }
> > 
> > wdyt?
> 
> > WebCore/storage/SQLTransactionSync.cpp:208
> >  +      return postflightExceptionCode;
> > Ditto shouldn't we be doing this in the transaction.
> 
> done.
>
> i think doing it atomically is the right way to do it, so in the code i moved the "check version" and "update
> version" steps inside the transaction. i'll send an email to hixie and ask him for a clarification (and we
> should probably make it more clear in the spec too).
Comment 77 Michael Nordman 2010-07-14 11:39:14 PDT
Oh... nevermind :)

120	    if (!setVersionInDatabase(newVersion)) {
121	        ec = SQLException::UNKNOWN_ERR;
122	        return;
123	    }
124	
125	    if ((ec = transaction->commit()))
126	        return;
127	
128	    setExpectedVersion(newVersion);
Comment 78 Dumitru Daniliuc 2010-07-22 01:47:39 PDT
Created attachment 62275 [details]
patch #4: interrupt all DB operations when the worker is shutting down
Comment 79 Ojan Vafai 2010-07-22 12:19:54 PDT
(In reply to comment #78)
> Created an attachment (id=62275) [details]
> patch #4: interrupt all DB operations when the worker is shutting down

Can you move this patch to a separate bug? This could serve as an umbrella bug, but for the sake of committing/reviewing, each bug should have one patch.
Comment 80 Dumitru Daniliuc 2010-07-22 13:15:25 PDT
Comment on attachment 62275 [details]
patch #4: interrupt all DB operations when the worker is shutting down

moving patch #4 to bug 42843, and closing this bug.