Bug 157843 – Modern IDB: Add support for server side closing of open database connections

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Brady Eidson]

Modern IDB: Add support for server side closing of open database connections

This is to support https://bugs.webkit.org/show_bug.cgi?id=157626

[Brady Eidson]

When the IDBServer decides to close a database connection itself, a lot of cleanup has to take place.

The server has to:
- Error out all requests it knows about
- Abort all transactions it knows about
- Tell the connection that it is being closed
- Wait for the connection to acknowledge that it was closed on the server

The client has to:
- Error out all requests it hasn't sent to the server
- Abort all transactions that haven't already been aborted by the server
- Send acknowledgement to the server that it has been closed.

Additionally, because of the asynchronous nature of requests being "in flight" between the client and server, a few design assumptions change, requiring changing some asserts and null checks to be graceful failures, etc etc.

[Brady Eidson]

Created attachment 279266 [details]
Patch v1

[WebKit Commit Bot]

Attachment 279266 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:488:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:510:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/Modules/indexeddb/server/IDBServer.h:101:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/Modules/indexeddb/server/IDBServer.h:134:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 279266 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=279266&action=review

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:1087
> +    Vector<RefPtr<IDBClient::TransactionOperation>> operations;
> +    operations.reserveInitialCapacity(m_transactionOperationMap.size());
> +    for (auto& operation : m_transactionOperationMap.values())
> +        operations.uncheckedAppend(operation);

copyValuesToVector

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:495
> +    // If the modification time is in the future, don't both doing anything.
> +    if (modificationTime > std::chrono::system_clock::now()) {

Is this really a common case worth optimizing for?

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:519
> +    for (auto* connection : m_databaseConnections.values()) {
> +        const auto& identifier = connection->database().identifier();
> +        for (auto& origin : origins) {

This could get slow if there are a lot of origins and connections.  Is there any way to use a HashSet or something?

[Brady Eidson]

(In reply to comment #4)
> Comment on attachment 279266 [details]
> Patch v1
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=279266&action=review
> 
> > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:1087
> > +    Vector<RefPtr<IDBClient::TransactionOperation>> operations;
> > +    operations.reserveInitialCapacity(m_transactionOperationMap.size());
> > +    for (auto& operation : m_transactionOperationMap.values())
> > +        operations.uncheckedAppend(operation);
> 
> copyValuesToVector

Yup.

> 
> > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:495
> > +    // If the modification time is in the future, don't both doing anything.
> > +    if (modificationTime > std::chrono::system_clock::now()) {
> 
> Is this really a common case worth optimizing for?

The commonality is indeterminate, since this is directly exposed as API.
The optimization definitely seems worthwhile because skipping it would involve performing a whole bunch of i/o just to do nothing.
 
> > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:519
> > +    for (auto* connection : m_databaseConnections.values()) {
> > +        const auto& identifier = connection->database().identifier();
> > +        for (auto& origin : origins) {
> 
> This could get slow if there are a lot of origins and connections.  Is there
> any way to use a HashSet or something?

The common case for known usage of this API is 1 origin.

For 1 origin, the Vector impl is faster and more space efficient than a HashSet, even before allowing for the construction of the HashSet itself.

I agree that if 1000's of origins came in the HashSet would be clearly superior, but in that case the entire operation will be very long running.

Especially since we have no reason to believe that happens, combined with the fact that this is already not blocking the main thread of either the Web or UI process, I think that is a premature optimization.

[Brady Eidson]

The patch is obviously causing havoc with the imported w3c tests (which I hadn't run locally, as I always assume storage/indexeddb is a better suite anyways)

Checking that locally...

[Brady Eidson]

(In reply to comment #6)
> The patch is obviously causing havoc with the imported w3c tests (which I
> hadn't run locally, as I always assume storage/indexeddb is a better suite
> anyways)
> 
> Checking that locally...

Never mind, nothing to see here.

Charge forward.

[Brady Eidson]

Created attachment 279278 [details]
Patch for landing

[WebKit Commit Bot]

Attachment 279278 [details] did not pass style-queue:


ERROR: Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:488:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:510:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/Modules/indexeddb/server/IDBServer.h:101:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/Modules/indexeddb/server/IDBServer.h:134:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Commit Bot]

Comment on attachment 279278 [details]
Patch for landing

Clearing flags on attachment: 279278

Committed r201098: <http://trac.webkit.org/changeset/201098>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.