Closed Bug 1454309 Opened 6 years ago Closed 6 years ago

Explicitly close storage connection before dispatching to connection pool

Categories

(Core :: Storage: IndexedDB, enhancement, P2)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: janv, Assigned: janv)

Details

Attachments

(1 file)

      No description provided.
Attached patch patchSplinter Review
I was getting a lot of debug messages about connection thread receiving STORAGE_BUSY error. I found out that OpenDatabaseOp::DoDatabaseWork() dispatches to the connection pool and storage connection is then implicitly closed. However the closing may last long time, so the connection thread will get the busy error. This isn't a big problem in the end, stuff works as expected, but things may run slower and getting a lot of these messages in the console is not very useful.
Assignee: nobody → jvarga
Status: NEW → ASSIGNED
Attachment #8968120 - Flags: review?(bugmail)
Priority: -- → P2
Comment on attachment 8968120 [details] [diff] [review]
patch

Review of attachment 8968120 [details] [diff] [review]:
-----------------------------------------------------------------

Restating:
- We've been seeing STORAGE_BUSY errors for some time in logs when IDB is trying to open the underlying SQLite databases.
- There's an odd behavior where OpenDatabaseOp gets its own low level mozIStorageConnection that is different from the ConnectionPool's in ConnectionPool::GetOrCreateConnection (which then gets wrapped into a DatabaseConnection).  This is at the root of the problem.  The odd behavior is almost certainly due to the mozStorage API insisting that its connections be opened and closed on the same thread, with the OpenDatabaseOp happening on the QuotaManager IO thread and the ConnectionPool call happening on one of its pool threads.  Bug 1121282 tracks improving mozStorage to allow optimizing this.
- If the OpenDatabaseOp dispatches itself back to its owning event target before closing the connection, there's a potential race between closing the connection on the QuotaManager IO thread and (re)opening the database on the connection pool thread.  Thus this fix is about making sure that the Close() explicitly happens.  The close() can take a while for a variety of I/O reasons, but also because WAL checkpointing occurs at database close time.  
- AutoSetProgressHandler is present in this patch because it was previously relying on its destructor to clean things up, which would happen shortly before the mozStorage connection would get cleaned up by its destructor.  Since we're now triggering the close earlier but haven't change the ASPH scope's lifetime, the ASPH cleanup needs to be explicitly triggered which means its logic needs to be moved into a helper.
  - AutoSetProgressHandler exists to associate the set the given operation with the underlying mozStorage connection as its progress handler so that a long-running SQLite operation can be aborted from inside the otherwise-blocking sqlite3_step/related calls.  (Note that since bug 1320301 mozStorage has supported the sqlite3_interrupt() mechanism which is more efficient, but only for async connections.  However, IDB could use such a mechanism too.)
Attachment #8968120 - Flags: review?(bugmail) → review+
Right, couldn't be written any better.
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/30ef2759d593
Explicitly close storage connection before dispatching to connection pool; r=asuth
https://hg.mozilla.org/mozilla-central/rev/30ef2759d593
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.