opening transactions only on write operations which enforces exclusive lock only there to improve concurrency, custom handling of sync request timeouts, #1093, #1018

src/public/app/services/sync.js

@@ -8,8 +8,8 @@ async function syncNow() {
         toastService.showMessage("Sync finished successfully.");
     }
     else {
-        if (result.message.length > 50) {
-            result.message = result.message.substr(0, 50);
+        if (result.message.length > 100) {
+            result.message = result.message.substr(0, 100);
         }
 
         toastService.showError("Sync failed: " + result.message);

src/public/app/services/utils.js

@@ -316,11 +316,11 @@ function dynamicRequire(moduleName) {
     }
 }
 
-function timeLimit(cb, limitMs) {
+function timeLimit(promise, limitMs) {
     return new Promise((res, rej) => {
         let resolved = false;
 
-        cb().then(() => {
+        promise.then(() => {
             resolved = true;
 
             res();
@@ -328,7 +328,7 @@ function timeLimit(cb, limitMs) {
 
         setTimeout(() => {
             if (!resolved) {
-                rej('Process exceeded time limit ' + limitMs);
+                rej(new Error('Process exceeded time limit ' + limitMs));
             }
         }, limitMs);
     });

src/public/app/services/ws.js

@@ -157,7 +157,7 @@ async function consumeSyncData() {
         const nonProcessedSyncRows = allSyncRows.filter(sync => !processedSyncIds.has(sync.id));
 
         try {
-            await utils.timeLimit(async () => await processSyncRows(nonProcessedSyncRows), 5000);
+            await utils.timeLimit(processSyncRows(nonProcessedSyncRows), 5000);
         }
         catch (e) {
             logError(`Encountered error ${e.message}: ${e.stack}, reloading frontend.`);

src/public/app/widgets/global_menu.js

@@ -35,7 +35,7 @@ const TPL = `
 
             <a class="dropdown-item sync-now-button" title="Trigger sync">
                 <span class="bx bx-refresh"></span>
-                Sync (<span id="outstanding-syncs-count">0</span>)
+                Sync now (<span id="outstanding-syncs-count">0</span>)
             </a>
 
             <a class="dropdown-item" data-trigger-command="openNewWindow">

src/routes/api/sync.js

@@ -55,6 +55,8 @@ async function checkSync() {
 }
 
 async function syncNow() {
+    log.info("Received request to trigger sync now.");
+
     return await syncService.sync();
 }
 

src/services/request.js

@@ -40,7 +40,7 @@ function exec(opts) {
                 host: parsedTargetUrl.hostname,
                 port: parsedTargetUrl.port,
                 path: parsedTargetUrl.path,
-                timeout: opts.timeout,
+                timeout: opts.timeout, // works only for node.js client
                 headers,
                 agent: proxyAgent
             });
@@ -104,7 +104,7 @@ async function getImage(imageUrl) {
                 host: parsedTargetUrl.hostname,
                 port: parsedTargetUrl.port,
                 path: parsedTargetUrl.path,
-                timeout: opts.timeout,
+                timeout: opts.timeout, // works only for node client
                 headers: {},
                 agent: proxyAgent
             });

src/services/setup.js

@@ -6,6 +6,7 @@ const optionService = require('./options');
 const syncOptions = require('./sync_options');
 const request = require('./request');
 const appInfo = require('./app_info');
+const utils = require('./utils');
 
 async function hasSyncServerSchemaAndSeed() {
     const response = await requestToSyncServer('GET', '/api/setup/status');
@@ -43,13 +44,15 @@ async function sendSeedToSyncServer() {
 }
 
 async function requestToSyncServer(method, path, body = null) {
-    return await request.exec({
+    const timeout = await syncOptions.getSyncTimeout();
+
+    return utils.timeLimit(request.exec({
         method,
         url: await syncOptions.getSyncServerHost() + path,
         body,
         proxy: await syncOptions.getSyncProxy(),
-        timeout: await syncOptions.getSyncTimeout()
-    });
+        timeout: timeout
+    }), timeout);
 }
 
 async function setupSyncFromSyncServer(syncServerHost, syncProxy, username, password) {

src/services/sql.js

@@ -64,15 +64,15 @@ async function upsert(tableName, primaryKey, rec) {
 }
 
 async function beginTransaction() {
-    return await execute("BEGIN");
+    return await dbConnection.run("BEGIN");
 }
 
 async function commit() {
-    return await execute("COMMIT");
+    return await dbConnection.run("COMMIT");
 }
 
 async function rollback() {
-    return await execute("ROLLBACK");
+    return await dbConnection.run("ROLLBACK");
 }
 
 async function getRow(query, params = []) {
@@ -150,6 +150,8 @@ async function getColumn(query, params = []) {
 }
 
 async function execute(query, params = []) {
+    await startTransactionIfNecessary();
+
     return await wrap(async db => db.run(query, ...params), query);
 }
 
@@ -158,11 +160,15 @@ async function executeNoWrap(query, params = []) {
 }
 
 async function executeMany(query, params) {
+    await startTransactionIfNecessary();
+
     // essentially just alias
     await getManyRows(query, params);
 }
 
 async function executeScript(query) {
+    await startTransactionIfNecessary();
+
     return await wrap(async db => db.exec(query), query);
 }
 
@@ -199,61 +205,65 @@ async function wrap(func, query) {
     }
 }
 
+// true if transaction is active globally.
+// cls.namespace.get('isTransactional') OTOH indicates active transaction in active CLS
 let transactionActive = false;
+// resolves when current transaction ends with either COMMIT or ROLLBACK
 let transactionPromise = null;
+let transactionPromiseResolve = null;
 
-async function transactional(func) {
-    if (cls.namespace.get('isInTransaction')) {
-        return await func();
+async function startTransactionIfNecessary() {
+    if (!cls.namespace.get('isTransactional')
+        || cls.namespace.get('isInTransaction')) {
+        return;
     }
 
     while (transactionActive) {
         await transactionPromise;
     }
 
-    let ret = null;
-    const thisError = new Error(); // to capture correct stack trace in case of exception
-
-    transactionActive = true;
-    transactionPromise = new Promise(async (resolve, reject) => {
-        try {
     await beginTransaction();
-
     cls.namespace.set('isInTransaction', true);
+    transactionActive = true;
+    transactionPromise = new Promise(res => transactionPromiseResolve = res);
+}
 
-            ret = await func();
+async function transactional(func) {
+    // if the CLS is already transactional then the whole transaction is handled by higher level transactional() call
+    if (cls.namespace.get('isTransactional')) {
+        return await func();
+    }
 
+    cls.namespace.set('isTransactional', true); // we will need a transaction if there's a write operation
+
+    try {
+        const ret = await func();
+
+        if (cls.namespace.get('isInTransaction')) {
             await commit();
 
             // note that sync rows sent from this action will be sent again by scheduled periodic ping
             require('./ws.js').sendPingToAllClients();
 
             transactionActive = false;
-            resolve();
-
-            setTimeout(() => require('./ws').sendPingToAllClients(), 50);
-        }
-        catch (e) {
-            if (transactionActive) {
-                log.error("Error executing transaction, executing rollback. Inner stack: " + e.stack + "\nOutside stack: " + thisError.stack);
-
-                await rollback();
-
-                transactionActive = false;
-            }
-
-            reject(e);
-        }
-        finally {
             cls.namespace.set('isInTransaction', false);
-        }
-    });
-
-    if (transactionActive) {
-        await transactionPromise;
+            transactionPromiseResolve();
         }
 
         return ret;
+    }
+    catch (e) {
+        if (transactionActive) {
+            await rollback();
+
+            transactionActive = false;
+            cls.namespace.set('isInTransaction', false);
+            // resolving since this is just semaphore for allowing another write transaction to proceed
+            transactionPromiseResolve();
+        }
+
+        throw e;
+    }
 }
 
 module.exports = {

src/services/sync.js

@@ -70,7 +70,7 @@ async function sync() {
             };
         }
         else {
-            log.info("sync failed: " + e.message + e.stack);
+            log.info("sync failed: " + e.message + "\nstack: " + e.stack);
 
             return {
                 success: false,
@@ -97,7 +97,6 @@ async function doLogin() {
     const hash = utils.hmac(documentSecret, timestamp);
 
     const syncContext = { cookieJar: {} };
-
     const resp = await syncRequest(syncContext, 'POST', '/api/login/sync', {
         timestamp: timestamp,
         syncVersion: appInfo.syncVersion,
@@ -259,14 +258,18 @@ async function checkContentHash(syncContext) {
 }
 
 async function syncRequest(syncContext, method, requestPath, body) {
-    return await request.exec({
+    const timeout = await syncOptions.getSyncTimeout();
+
+    const opts = {
         method,
         url: await syncOptions.getSyncServerHost() + requestPath,
         cookieJar: syncContext.cookieJar,
-        timeout: await syncOptions.getSyncTimeout(),
+        timeout: timeout,
         body,
         proxy: proxyToggle ? await syncOptions.getSyncProxy() : null
-    });
+    };
+
+    return await utils.timeLimit(request.exec(opts), timeout);
 }
 
 const primaryKeys = {

src/services/utils.js

@@ -217,6 +217,24 @@ function formatDownloadTitle(filename, type, mime) {
     }
 }
 
+function timeLimit(promise, limitMs) {
+    return new Promise((res, rej) => {
+        let resolved = false;
+
+        promise.then(() => {
+            resolved = true;
+
+            res();
+        });
+
+        setTimeout(() => {
+            if (!resolved) {
+                rej(new Error('Process exceeded time limit ' + limitMs));
+            }
+        }, limitMs);
+    });
+}
+
 module.exports = {
     randomSecureToken,
     randomString,
@@ -245,5 +263,6 @@ module.exports = {
     isStringNote,
     quoteRegex,
     replaceAll,
-    formatDownloadTitle
+    formatDownloadTitle,
+    timeLimit
 };