2009-09-22 Dumitru Daniliuc <dumi@chromium.org>
authoreric@webkit.org <eric@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 22 Sep 2009 22:24:56 +0000 (22:24 +0000)
committereric@webkit.org <eric@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 22 Sep 2009 22:24:56 +0000 (22:24 +0000)
        Reviewed by Dimitri Glazkov.

        1. Adding two tests for the transaction coordinator.
        2. Fixing an incorrect <head> tag in some tests.

        https://bugs.webkit.org/show_bug.cgi?id=29115

        * storage/multiple-transactions-on-different-handles.html: Fixed a
        <head> tag (should've been </head>).
        * storage/open-database-while-transaction-in-progress.html: Fixed
        a <head> tag (should've been </head>).
        * storage/read-and-write-transactions-dont-run-together-expected.txt: Added.
        * storage/read-and-write-transactions-dont-run-together.html: Added.
        * storage/read-transactions-running-concurrently-expected.txt: Added.
        * storage/read-transactions-running-concurrently.html: Added.
        * storage/test-authorizer.html: Fixed a <head> tag (should've been
        </head>).
2009-09-22  Dumitru Daniliuc  <dumi@chromium.org>

        Reviewed by Dimitri Glazkov.

        Changing the transaction coordinator to (re-)allow multiple read
        transactions on the same database to run concurrently (without
        risking a deadlock this time).

        https://bugs.webkit.org/show_bug.cgi?id=29115

        Tests: storage/read-and-write-transactions-dont-run-together.html
               storage/read-transactions-running-concurrently.html

        * storage/SQLTransaction.h:
        (WebCore::SQLTransaction::isReadOnly): Returns the type of the
        transaction.
        * storage/SQLTransactionCoordinator.cpp:
        (WebCore::SQLTransactionCoordinator::acquireLock): Changed to
        allow multiple read transactions on the same DB to run
        concurrently.
        (WebCore::SQLTransactionCoordinator::releaseLock): Changed to
        allow multiple read transactions on the same DB to run
        concurrently.
        (WebCore::SQLTransactionCoordinator::shutdown): Renamed the map.
        * storage/SQLTransactionCoordinator.h:

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@48653 268f45cc-cd09-0410-ab3c-d52691b4dbfc

13 files changed:
LayoutTests/ChangeLog
LayoutTests/storage/multiple-transactions-on-different-handles.html
LayoutTests/storage/open-database-while-transaction-in-progress.html
LayoutTests/storage/read-and-write-transactions-dont-run-together-expected.txt [new file with mode: 0644]
LayoutTests/storage/read-and-write-transactions-dont-run-together.html [new file with mode: 0644]
LayoutTests/storage/read-transactions-running-concurrently-expected.txt [new file with mode: 0644]
LayoutTests/storage/read-transactions-running-concurrently.html [new file with mode: 0644]
LayoutTests/storage/test-authorizer.html
WebCore/ChangeLog
WebCore/storage/SQLTransaction.cpp
WebCore/storage/SQLTransaction.h
WebCore/storage/SQLTransactionCoordinator.cpp
WebCore/storage/SQLTransactionCoordinator.h

index 55c152f..aafeee5 100644 (file)
@@ -1,3 +1,23 @@
+2009-09-22  Dumitru Daniliuc  <dumi@chromium.org>
+
+        Reviewed by Dimitri Glazkov.
+
+        1. Adding two tests for the transaction coordinator.
+        2. Fixing an incorrect <head> tag in some tests.
+
+        https://bugs.webkit.org/show_bug.cgi?id=29115
+
+        * storage/multiple-transactions-on-different-handles.html: Fixed a
+        <head> tag (should've been </head>).
+        * storage/open-database-while-transaction-in-progress.html: Fixed
+        a <head> tag (should've been </head>).
+        * storage/read-and-write-transactions-dont-run-together-expected.txt: Added.
+        * storage/read-and-write-transactions-dont-run-together.html: Added.
+        * storage/read-transactions-running-concurrently-expected.txt: Added.
+        * storage/read-transactions-running-concurrently.html: Added.
+        * storage/test-authorizer.html: Fixed a <head> tag (should've been
+        </head>).
+
 2009-09-22  Shinichiro Hamaji  <hamaji@chromium.org>
 
         Rubber-stamped by Eric Seidel.
index 9a39e15..bcdab82 100644 (file)
@@ -89,7 +89,7 @@ function runTest() {
     } catch(err) {}
 }
 </script>
-<head>
+</head>
 <body onload="runTest();">
 This is a test to see if queueing up multiple transactions on different handles to the same database results in a deadlock.<br>
 </body>
index 6f727ef..faa63b6 100644 (file)
@@ -64,7 +64,7 @@ function runTest()
     } catch(err) {}
 }
 </script>
-<head>
+</head>
 <body onload="runTest();">
 This is a test to see if opening a database while a transaction is running on a different handle to the same database results in a deadlock.<br>
 </body>
diff --git a/LayoutTests/storage/read-and-write-transactions-dont-run-together-expected.txt b/LayoutTests/storage/read-and-write-transactions-dont-run-together-expected.txt
new file mode 100644 (file)
index 0000000..cc8979c
--- /dev/null
@@ -0,0 +1,12 @@
+This test tests that read and write transactions on different handles to the same database don't run together.
+Transaction successful.
+Transaction successful.
+Transaction successful.
+Transaction successful.
+Transaction successful.
+Transaction successful.
+Transaction successful.
+Transaction successful.
+Transaction successful.
+Transaction successful.
+
diff --git a/LayoutTests/storage/read-and-write-transactions-dont-run-together.html b/LayoutTests/storage/read-and-write-transactions-dont-run-together.html
new file mode 100644 (file)
index 0000000..a7565b5
--- /dev/null
@@ -0,0 +1,103 @@
+<html>
+<head>
+<script>
+
+function log(message)
+{
+    document.body.innerHTML += message + "<br>";
+}
+
+function terminateTest()
+{
+    if (window.layoutTestController)
+        layoutTestController.notifyDone();
+}
+
+function openTestDatabase()
+{
+    return openDatabase("ReadAndWriteTransactionsDontRunTogetherTest",
+                        "1.0",
+                        "Test to make sure that read and write transactions on different DB handles to the same DB don't run at the same time.",
+                        32768);
+}
+
+var readTransactionsInProgress = 0;
+var writeTransactionsInProgress = 0;
+var totalTransactions = 0;
+var finishedTransactions = 0;
+
+function runTransaction(db, readOnly)
+{
+    var transactionFunction = (readOnly ? db.readTransaction : db.transaction);
+    transactionFunction.call(db, function(tx) {
+            if (readOnly) {
+                if (writeTransactionsInProgress != 0) {
+                    log("Read transaction starting while write transaction in progress.");
+                    terminateTest();
+                }
+                readTransactionsInProgress++;
+            } else {
+                if ((readTransactionsInProgress != 0) || (writeTransactionsInProgress != 0)) {
+                    log("Write transaction starting while another transaction in progress.");
+                    terminateTest();
+                }
+                writeTransactionsInProgress++;
+            }
+            tx.executeSql("SELECT * FROM Test;");
+        }, function(error) {
+            log((readOnly ? "Read" : "Write") + " transaction failed: " + error.message);
+            terminateTest();
+        }, function() {
+             finishedTransactions++;
+             if (readOnly)
+                 readTransactionsInProgress--;
+             else
+                 writeTransactionsInProgress--;
+             log("Transaction successful.");
+             if ((finishedTransactions == totalTransactions) && (readTransactionsInProgress == 0) && (writeTransactionsInProgress == 0))
+                 terminateTest();
+        });
+}
+
+function runReadAndWriteTransactions(db1, db2, db3)
+{
+    totalTransactions = 10;
+    finishedTransactions = 0;
+    runTransaction(db1, true);
+    runTransaction(db2, true);
+    runTransaction(db1, false);
+    runTransaction(db1, true);
+    runTransaction(db2, true);
+    runTransaction(db3, true);
+    runTransaction(db1, false);
+    runTransaction(db2, false);
+    runTransaction(db1, true);
+    runTransaction(db3, true);
+}
+
+function runTest() {
+    if (window.layoutTestController) {
+        layoutTestController.dumpAsText();
+        layoutTestController.waitUntilDone();
+    }
+
+    try {
+        var db1 = openTestDatabase();
+        var db2 = openTestDatabase();
+        var db3 = openTestDatabase();
+        db1.transaction(function(tx) {
+                tx.executeSql("CREATE TABLE IF NOT EXISTS Test (Foo int);");
+            }, function(error) {
+                log("Cannot create the Test table: " + error.message);
+                terminateTest();
+            }, function() {
+                runReadAndWriteTransactions(db1, db2, db3);
+            });
+     } catch(err) {}
+}
+</script>
+</head>
+<body onload="runTest();">
+This test tests that read and write transactions on different handles to the same database don't run together.<br>
+</body>
+</html>
diff --git a/LayoutTests/storage/read-transactions-running-concurrently-expected.txt b/LayoutTests/storage/read-transactions-running-concurrently-expected.txt
new file mode 100644 (file)
index 0000000..f58d2df
--- /dev/null
@@ -0,0 +1,3 @@
+This test tests that two read-only transactions on different handles to the same database run concurrently.
+Read transactions running concurrently.
+
diff --git a/LayoutTests/storage/read-transactions-running-concurrently.html b/LayoutTests/storage/read-transactions-running-concurrently.html
new file mode 100644 (file)
index 0000000..b2d862c
--- /dev/null
@@ -0,0 +1,67 @@
+<html>
+<head>
+<script>
+
+function log(message)
+{
+    document.body.innerHTML += message + "<br>";
+}
+
+function terminateTest()
+{
+    if (window.layoutTestController)
+        layoutTestController.notifyDone();
+}
+
+function openTestDatabase()
+{
+    return openDatabase("ReadTransactionsRunningConcurrentlyTest",
+                        "1.0",
+                        "Test to make sure that multiple read transactions on different DB handles to the same DB run concurrently.",
+                        32768);
+}
+
+var readTransactionsInProgress = 0;
+
+function runReadTransaction(db)
+{
+    db.readTransaction(function(tx) {
+            readTransactionsInProgress++;
+        }, function(error) {
+            log("Read transaction failed: " + error.message);
+            terminateTest();
+        }, function() {
+            if (readTransactionsInProgress == 2)
+                log("Read transactions running concurrently.");
+            readTransactionsInProgress--;
+            if (readTransactionsInProgress == 0)
+                terminateTest();
+        });
+}
+
+function runTest() {
+    if (window.layoutTestController) {
+        layoutTestController.dumpAsText();
+        layoutTestController.waitUntilDone();
+    }
+
+    try {
+        var db1 = openTestDatabase();
+        var db2 = openTestDatabase();
+        db1.transaction(function(tx) {
+                tx.executeSql("CREATE TABLE IF NOT EXISTS Test (Foo int);");
+            }, function(error) {
+                log("Cannot create the Test table: " + error.message);
+                terminateTest();
+            }, function() {
+                runReadTransaction(db1);
+                runReadTransaction(db2);
+            });
+     } catch(err) { log(err); }
+}
+</script>
+</head>
+<body onload="runTest();">
+This test tests that two read-only transactions on different handles to the same database run concurrently.<br>
+</body>
+</html>
index 715d9da..57785a6 100644 (file)
@@ -168,7 +168,7 @@ function runTest()
     } catch(err) {}
 }
 </script>
-<head>
+</head>
 <body onload="runTest();">
 This test tests the database authorizer.<br>
 </body>
index b3f2737..8b90aac 100644 (file)
@@ -1,3 +1,29 @@
+2009-09-22  Dumitru Daniliuc  <dumi@chromium.org>
+
+        Reviewed by Dimitri Glazkov.
+
+        Changing the transaction coordinator to (re-)allow multiple read
+        transactions on the same database to run concurrently (without
+        risking a deadlock this time).
+
+        https://bugs.webkit.org/show_bug.cgi?id=29115
+
+        Tests: storage/read-and-write-transactions-dont-run-together.html
+               storage/read-transactions-running-concurrently.html
+
+        * storage/SQLTransaction.h:
+        (WebCore::SQLTransaction::isReadOnly): Returns the type of the
+        transaction.
+        * storage/SQLTransactionCoordinator.cpp:
+        (WebCore::SQLTransactionCoordinator::acquireLock): Changed to
+        allow multiple read transactions on the same DB to run
+        concurrently.
+        (WebCore::SQLTransactionCoordinator::releaseLock): Changed to
+        allow multiple read transactions on the same DB to run
+        concurrently.
+        (WebCore::SQLTransactionCoordinator::shutdown): Renamed the map.
+        * storage/SQLTransactionCoordinator.h:
+
 2009-09-22  Peter Kasting  <pkasting@google.com>
 
         Reviewed by David Levin.
index 4ce65f6..dabbac2 100644 (file)
@@ -207,7 +207,7 @@ void SQLTransaction::performPendingCallback()
 
 void SQLTransaction::acquireLock()
 {
-    m_database->transactionCoordinator()->acquireLock(this, m_readOnly);
+    m_database->transactionCoordinator()->acquireLock(this);
 }
 
 void SQLTransaction::lockAcquired()
index 0586cc5..6d6a8d7 100644 (file)
@@ -79,6 +79,7 @@ public:
     void performPendingCallback();
 
     Database* database() { return m_database.get(); }
+    bool isReadOnly() { return m_readOnly; }
 
 private:
     SQLTransaction(Database*, PassRefPtr<SQLTransactionCallback>, PassRefPtr<SQLTransactionErrorCallback>,
index a42734f..30b0c4a 100644 (file)
@@ -36,8 +36,8 @@
 #include "SQLTransaction.h"
 #include <wtf/Deque.h>
 #include <wtf/HashMap.h>
+#include <wtf/HashSet.h>
 #include <wtf/RefPtr.h>
-#include <wtf/UnusedParam.h>
 
 namespace WebCore {
 
@@ -48,58 +48,67 @@ static String getDatabaseIdentifier(SQLTransaction* transaction)
     return database->stringIdentifier();
 }
 
-void SQLTransactionCoordinator::acquireLock(SQLTransaction* transaction, bool readOnly)
+void SQLTransactionCoordinator::processPendingTransactions(CoordinationInfo& info)
 {
-    UNUSED_PARAM(readOnly);
+    if (info.activeWriteTransaction || info.pendingTransactions.isEmpty())
+        return;
 
+    RefPtr<SQLTransaction> firstPendingTransaction = info.pendingTransactions.first();
+    if (firstPendingTransaction->isReadOnly()) {
+        do {
+            firstPendingTransaction = info.pendingTransactions.first();
+            info.pendingTransactions.removeFirst();
+            info.activeReadTransactions.add(firstPendingTransaction);
+            firstPendingTransaction->lockAcquired();
+        } while (!info.pendingTransactions.isEmpty() && info.pendingTransactions.first()->isReadOnly());
+    } else if (info.activeReadTransactions.isEmpty()) {
+        info.pendingTransactions.removeFirst();
+        info.activeWriteTransaction = firstPendingTransaction;
+        firstPendingTransaction->lockAcquired();
+    }
+}
+
+void SQLTransactionCoordinator::acquireLock(SQLTransaction* transaction)
+{
     String dbIdentifier = getDatabaseIdentifier(transaction);
 
-    TransactionsHashMap::iterator it = m_pendingTransactions.find(dbIdentifier);
-    if (it == m_pendingTransactions.end()) {
+    CoordinationInfoMap::iterator coordinationInfoIterator = m_coordinationInfoMap.find(dbIdentifier);
+    if (coordinationInfoIterator == m_coordinationInfoMap.end()) {
         // No pending transactions for this DB
-        TransactionsQueue pendingTransactions;
-        pendingTransactions.append(transaction);
-        m_pendingTransactions.add(dbIdentifier, pendingTransactions);
-
-        // Start the transaction
-        transaction->lockAcquired();
-    } else {
-        // Another transaction is running on this DB; put this one in the queue
-        TransactionsQueue& pendingTransactions = it->second;
-        pendingTransactions.append(transaction);
+        coordinationInfoIterator = m_coordinationInfoMap.add(dbIdentifier, CoordinationInfo()).first;
     }
+
+    CoordinationInfo& info = coordinationInfoIterator->second;
+    info.pendingTransactions.append(transaction);
+    processPendingTransactions(info);
 }
 
 void SQLTransactionCoordinator::releaseLock(SQLTransaction* transaction)
 {
-    if (m_pendingTransactions.isEmpty())
+    if (m_coordinationInfoMap.isEmpty())
         return;
 
     String dbIdentifier = getDatabaseIdentifier(transaction);
 
-    TransactionsHashMap::iterator it = m_pendingTransactions.find(dbIdentifier);
-    ASSERT(it != m_pendingTransactions.end());
-    TransactionsQueue& pendingTransactions = it->second;
-    ASSERT(!pendingTransactions.isEmpty());
+    CoordinationInfoMap::iterator coordinationInfoIterator = m_coordinationInfoMap.find(dbIdentifier);
+    ASSERT(coordinationInfoIterator != m_coordinationInfoMap.end());
+    CoordinationInfo& info = coordinationInfoIterator->second;
 
-    // 'transaction' should always be the first transaction in this queue
-    ASSERT(pendingTransactions.first().get() == transaction);
-
-    // Remove 'transaction' from the queue of pending transactions
-    pendingTransactions.removeFirst();
-    if (pendingTransactions.isEmpty()) {
-        // No more pending transactions; delete dbIdentifier's queue
-        m_pendingTransactions.remove(it);
+    if (transaction->isReadOnly()) {
+        ASSERT(info.activeReadTransactions.contains(transaction));
+        info.activeReadTransactions.remove(transaction);
     } else {
-        // We have more pending transactions; start the next one
-        pendingTransactions.first()->lockAcquired();
+        ASSERT(info.activeWriteTransaction == transaction);
+        info.activeWriteTransaction = 0;
     }
+
+    processPendingTransactions(info);
 }
 
 void SQLTransactionCoordinator::shutdown()
 {
     // Clean up all pending transactions for all databases
-    m_pendingTransactions.clear();
+    m_coordinationInfoMap.clear();
 }
 
 }
index 08985cf..20cc863 100644 (file)
@@ -35,6 +35,7 @@
 #include "StringHash.h"
 #include <wtf/Deque.h>
 #include <wtf/HashMap.h>
+#include <wtf/HashSet.h>
 #include <wtf/RefPtr.h>
 
 namespace WebCore {
@@ -43,13 +44,21 @@ namespace WebCore {
 
     class SQLTransactionCoordinator {
     public:
-        void acquireLock(SQLTransaction*, bool readOnly);
+        void acquireLock(SQLTransaction*);
         void releaseLock(SQLTransaction*);
         void shutdown();
     private:
         typedef Deque<RefPtr<SQLTransaction> > TransactionsQueue;
-        typedef HashMap<String, TransactionsQueue> TransactionsHashMap;
-        TransactionsHashMap m_pendingTransactions;
+        struct CoordinationInfo {
+            TransactionsQueue pendingTransactions;
+            HashSet<RefPtr<SQLTransaction> > activeReadTransactions;
+            RefPtr<SQLTransaction> activeWriteTransaction;
+        };
+        // Maps database names to information about pending transactions
+        typedef HashMap<String, CoordinationInfo> CoordinationInfoMap;
+        CoordinationInfoMap m_coordinationInfoMap;
+
+        void processPendingTransactions(CoordinationInfo& info);
     };
 }