From 707f473b85bba3991e69718068b5d2abe315e522 Mon Sep 17 00:00:00 2001 From: Jiarui Zhang <91682445+jiaruiz717@users.noreply.github.com> Date: Mon, 22 Jul 2024 10:24:13 -0400 Subject: [PATCH 01/12] check if current user holds the write lock before uploading changes --- iModelJsNodeAddon/JsCloudSqlite.cpp | 46 +++++++++++++++++++++++++++-- 1 file changed, 44 insertions(+), 2 deletions(-) diff --git a/iModelJsNodeAddon/JsCloudSqlite.cpp b/iModelJsNodeAddon/JsCloudSqlite.cpp index 18070d96de6..9b35a6f20fe 100644 --- a/iModelJsNodeAddon/JsCloudSqlite.cpp +++ b/iModelJsNodeAddon/JsCloudSqlite.cpp @@ -195,6 +195,46 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { BeNapi::ThrowJsException(Env(), Utf8PrintfString("container [%s] is not locked for write access", m_containerId.c_str()).c_str()); } + void CheckWriteLockHeldByCurrentUser() { + RequireConnected(); + BeJsDocument lockedBy; + ReadWriteLock(lockedBy); + auto lockedByGuid = lockedBy[JSON_NAME(guid)].asString(); + auto expiresAt = DateTime::FromString(lockedBy[JSON_NAME(expires)].asString()); + auto lockedByUser = lockedBy[JSON_NAME(user)].asString(); + + // check if it's the same guid + if (lockedByGuid != m_cache->m_guid) { + // another user grabbed the write lock after the current user's write lock expiration time, disable current user from operating + BeNapi::ThrowJsException(Env(), "container is currently locked by another user"); + } else { + if (DateTime::CompareResult::EarlierThan == DateTime::Compare(expiresAt, GetServerTime())) { + // no other users grab the write lock even after current write lock expiration time + // re-grab the write lock for the current user + // m_lockExpireSeconds = std::min((int) (12*SECONDS_PER_HOUR), std::max((int)SECONDS_PER_HOUR, m_lockExpireSeconds)); + // BeJsDocument lockedByUpdate; + // lockedByUpdate[JSON_NAME(guid)] = lockedByGuid; + // lockedByUpdate[JSON_NAME(user)] = lockedByUser; + // lockedByUpdate[JSON_NAME(expires)] = GetServerDateString(m_lockExpireSeconds * 1000); + // Statement stmt; + // auto rc = stmt.Prepare(m_containerDb, "REPLACE INTO bcv_kv(value,name) VALUES(?,?)"); + // BeAssert(rc == BE_SQLITE_OK); + // stmt.BindText(1, lockedByUpdate.Stringify(), Statement::MakeCopy::Yes); + // stmt.BindText(2, writeLockName, Statement::MakeCopy::No); + // rc = stmt.Step(); + // if (rc == BE_SQLITE_DONE) + // rc = m_containerDb.TryExecuteSql("COMMIT"); + // if (rc != BE_SQLITE_OK) { + // if (rc == BE_SQLITE_IOERR_AUTH) + // BeNapi::ThrowJsException(Env(), "not authorized to obtain write lock", BE_SQLITE_AUTH); + // BeNapi::ThrowJsException(Env(), "cannot obtain write lock", rc); + // } + ResumeWriteLock(); + } + } + + } + void CallJsMemberFunc(Utf8CP funcName, std::vector const& args) { auto func = Value().Get(funcName); if (func.IsFunction()) @@ -256,7 +296,8 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { } Napi::Value UploadChanges(NapiInfoCR info) { - RequireWriteLock(); + // RequireWriteLock(); + CheckWriteLockHeldByCurrentUser(); return QueueWorker(info, [=]() { return CloudContainer::UploadChanges(); }); } @@ -571,7 +612,8 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { m_containerDb.TryExecuteSql("BEGIN"); CheckLock(); // throws if already locked by another user - m_lockExpireSeconds = std::min((int) (12*SECONDS_PER_HOUR), std::max((int)SECONDS_PER_HOUR, m_lockExpireSeconds)); + // m_lockExpireSeconds = std::min((int) (12*SECONDS_PER_HOUR), std::max((int)SECONDS_PER_HOUR, m_lockExpireSeconds)); + m_lockExpireSeconds = 5; BeJsDocument lockedBy; lockedBy[JSON_NAME(guid)] = m_cache->m_guid; lockedBy[JSON_NAME(user)] = user; From a2ec1d61d991609ad4a75bd70571f263a9e56936 Mon Sep 17 00:00:00 2001 From: Jiarui Zhang <91682445+jiaruiz717@users.noreply.github.com> Date: Tue, 23 Jul 2024 09:38:51 -0400 Subject: [PATCH 02/12] default time for write lock 5 seconds --- iModelJsNodeAddon/JsCloudSqlite.cpp | 27 +++------------------------ 1 file changed, 3 insertions(+), 24 deletions(-) diff --git a/iModelJsNodeAddon/JsCloudSqlite.cpp b/iModelJsNodeAddon/JsCloudSqlite.cpp index 9b35a6f20fe..73ff09222af 100644 --- a/iModelJsNodeAddon/JsCloudSqlite.cpp +++ b/iModelJsNodeAddon/JsCloudSqlite.cpp @@ -11,6 +11,7 @@ namespace IModelJsNative { enum : uint64_t { SECONDS_PER_MINUTE = 60, SECONDS_PER_HOUR = SECONDS_PER_MINUTE * 60, + FIVE_SECONDS = 5, }; /** adapted from sqlite code */ @@ -206,29 +207,9 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { // check if it's the same guid if (lockedByGuid != m_cache->m_guid) { // another user grabbed the write lock after the current user's write lock expiration time, disable current user from operating - BeNapi::ThrowJsException(Env(), "container is currently locked by another user"); + BeNapi::ThrowJsException(Env(), Utf8PrintfString("Container [%s] is currently locked by another user.", m_containerId.c_str()).c_str()); } else { if (DateTime::CompareResult::EarlierThan == DateTime::Compare(expiresAt, GetServerTime())) { - // no other users grab the write lock even after current write lock expiration time - // re-grab the write lock for the current user - // m_lockExpireSeconds = std::min((int) (12*SECONDS_PER_HOUR), std::max((int)SECONDS_PER_HOUR, m_lockExpireSeconds)); - // BeJsDocument lockedByUpdate; - // lockedByUpdate[JSON_NAME(guid)] = lockedByGuid; - // lockedByUpdate[JSON_NAME(user)] = lockedByUser; - // lockedByUpdate[JSON_NAME(expires)] = GetServerDateString(m_lockExpireSeconds * 1000); - // Statement stmt; - // auto rc = stmt.Prepare(m_containerDb, "REPLACE INTO bcv_kv(value,name) VALUES(?,?)"); - // BeAssert(rc == BE_SQLITE_OK); - // stmt.BindText(1, lockedByUpdate.Stringify(), Statement::MakeCopy::Yes); - // stmt.BindText(2, writeLockName, Statement::MakeCopy::No); - // rc = stmt.Step(); - // if (rc == BE_SQLITE_DONE) - // rc = m_containerDb.TryExecuteSql("COMMIT"); - // if (rc != BE_SQLITE_OK) { - // if (rc == BE_SQLITE_IOERR_AUTH) - // BeNapi::ThrowJsException(Env(), "not authorized to obtain write lock", BE_SQLITE_AUTH); - // BeNapi::ThrowJsException(Env(), "cannot obtain write lock", rc); - // } ResumeWriteLock(); } } @@ -296,7 +277,6 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { } Napi::Value UploadChanges(NapiInfoCR info) { - // RequireWriteLock(); CheckWriteLockHeldByCurrentUser(); return QueueWorker(info, [=]() { return CloudContainer::UploadChanges(); }); } @@ -612,8 +592,7 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { m_containerDb.TryExecuteSql("BEGIN"); CheckLock(); // throws if already locked by another user - // m_lockExpireSeconds = std::min((int) (12*SECONDS_PER_HOUR), std::max((int)SECONDS_PER_HOUR, m_lockExpireSeconds)); - m_lockExpireSeconds = 5; + m_lockExpireSeconds = std::min((int) (12*SECONDS_PER_HOUR), std::max((int)FIVE_SECONDS, m_lockExpireSeconds)); BeJsDocument lockedBy; lockedBy[JSON_NAME(guid)] = m_cache->m_guid; lockedBy[JSON_NAME(user)] = user; From b6ca45091d58289c2788af5f650b6341a30eed74 Mon Sep 17 00:00:00 2001 From: Jiarui Zhang <91682445+jiaruiz717@users.noreply.github.com> Date: Tue, 23 Jul 2024 16:21:26 -0400 Subject: [PATCH 03/12] write lock minimum duration is 5 min --- iModelJsNodeAddon/JsCloudSqlite.cpp | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/iModelJsNodeAddon/JsCloudSqlite.cpp b/iModelJsNodeAddon/JsCloudSqlite.cpp index 73ff09222af..731f1a321e7 100644 --- a/iModelJsNodeAddon/JsCloudSqlite.cpp +++ b/iModelJsNodeAddon/JsCloudSqlite.cpp @@ -11,7 +11,6 @@ namespace IModelJsNative { enum : uint64_t { SECONDS_PER_MINUTE = 60, SECONDS_PER_HOUR = SECONDS_PER_MINUTE * 60, - FIVE_SECONDS = 5, }; /** adapted from sqlite code */ @@ -203,7 +202,6 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { auto lockedByGuid = lockedBy[JSON_NAME(guid)].asString(); auto expiresAt = DateTime::FromString(lockedBy[JSON_NAME(expires)].asString()); auto lockedByUser = lockedBy[JSON_NAME(user)].asString(); - // check if it's the same guid if (lockedByGuid != m_cache->m_guid) { // another user grabbed the write lock after the current user's write lock expiration time, disable current user from operating @@ -213,7 +211,6 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { ResumeWriteLock(); } } - } void CallJsMemberFunc(Utf8CP funcName, std::vector const& args) { @@ -592,7 +589,7 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { m_containerDb.TryExecuteSql("BEGIN"); CheckLock(); // throws if already locked by another user - m_lockExpireSeconds = std::min((int) (12*SECONDS_PER_HOUR), std::max((int)FIVE_SECONDS, m_lockExpireSeconds)); + m_lockExpireSeconds = std::min((int) (12*SECONDS_PER_HOUR), std::max(5*((int)SECONDS_PER_MINUTE), m_lockExpireSeconds)); BeJsDocument lockedBy; lockedBy[JSON_NAME(guid)] = m_cache->m_guid; lockedBy[JSON_NAME(user)] = user; From 35f01fd017118730a226782e7d4cbebc0752b487 Mon Sep 17 00:00:00 2001 From: Jiarui Zhang <91682445+jiaruiz717@users.noreply.github.com> Date: Thu, 25 Jul 2024 11:48:58 -0400 Subject: [PATCH 04/12] replace RequireLock with CheckWriteLockHeldByCurrentUser --- iModelJsNodeAddon/JsCloudSqlite.cpp | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/iModelJsNodeAddon/JsCloudSqlite.cpp b/iModelJsNodeAddon/JsCloudSqlite.cpp index 731f1a321e7..c494b35febb 100644 --- a/iModelJsNodeAddon/JsCloudSqlite.cpp +++ b/iModelJsNodeAddon/JsCloudSqlite.cpp @@ -190,12 +190,6 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { } void RequireWriteLock() { - RequireConnected(); - if (!m_writeLockHeld) - BeNapi::ThrowJsException(Env(), Utf8PrintfString("container [%s] is not locked for write access", m_containerId.c_str()).c_str()); - } - - void CheckWriteLockHeldByCurrentUser() { RequireConnected(); BeJsDocument lockedBy; ReadWriteLock(lockedBy); @@ -274,7 +268,7 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { } Napi::Value UploadChanges(NapiInfoCR info) { - CheckWriteLockHeldByCurrentUser(); + RequireWriteLock(); return QueueWorker(info, [=]() { return CloudContainer::UploadChanges(); }); } From 5c8d75885252b85d66ac2895ec03a9e371039aca Mon Sep 17 00:00:00 2001 From: Jiarui Zhang <91682445+jiaruiz717@users.noreply.github.com> Date: Thu, 25 Jul 2024 14:43:00 -0400 Subject: [PATCH 05/12] resume write lock anyway --- iModelJsNodeAddon/JsCloudSqlite.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/iModelJsNodeAddon/JsCloudSqlite.cpp b/iModelJsNodeAddon/JsCloudSqlite.cpp index c494b35febb..fba4405dc9f 100644 --- a/iModelJsNodeAddon/JsCloudSqlite.cpp +++ b/iModelJsNodeAddon/JsCloudSqlite.cpp @@ -194,16 +194,16 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { BeJsDocument lockedBy; ReadWriteLock(lockedBy); auto lockedByGuid = lockedBy[JSON_NAME(guid)].asString(); - auto expiresAt = DateTime::FromString(lockedBy[JSON_NAME(expires)].asString()); - auto lockedByUser = lockedBy[JSON_NAME(user)].asString(); // check if it's the same guid if (lockedByGuid != m_cache->m_guid) { // another user grabbed the write lock after the current user's write lock expiration time, disable current user from operating BeNapi::ThrowJsException(Env(), Utf8PrintfString("Container [%s] is currently locked by another user.", m_containerId.c_str()).c_str()); } else { - if (DateTime::CompareResult::EarlierThan == DateTime::Compare(expiresAt, GetServerTime())) { - ResumeWriteLock(); - } + // Refresh the write lock for the current user no matter if the write lock expires + // 1. If the write lock expires, refresh it for the current user to operate further actions + // 2. If not, also refresh it in case the current user's write lock is about to expire when they call operations that require write lock to be present, and somehow expires during the operations + ResumeWriteLock(); + } } From c32f13881e48297481c3903552c2ed6109a6a921 Mon Sep 17 00:00:00 2001 From: Jiarui Zhang <91682445+jiaruiz717@users.noreply.github.com> Date: Mon, 29 Jul 2024 15:36:38 -0400 Subject: [PATCH 06/12] abandon changes if write lock was acquired by another user --- iModelJsNodeAddon/JsCloudSqlite.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/iModelJsNodeAddon/JsCloudSqlite.cpp b/iModelJsNodeAddon/JsCloudSqlite.cpp index fba4405dc9f..8c635f6ba1b 100644 --- a/iModelJsNodeAddon/JsCloudSqlite.cpp +++ b/iModelJsNodeAddon/JsCloudSqlite.cpp @@ -195,7 +195,7 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { ReadWriteLock(lockedBy); auto lockedByGuid = lockedBy[JSON_NAME(guid)].asString(); // check if it's the same guid - if (lockedByGuid != m_cache->m_guid) { + if (!lockedByGuid.Equals(m_cache->m_guid)) { // another user grabbed the write lock after the current user's write lock expiration time, disable current user from operating BeNapi::ThrowJsException(Env(), Utf8PrintfString("Container [%s] is currently locked by another user.", m_containerId.c_str()).c_str()); } else { @@ -234,7 +234,10 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { if (m_writeable) { ResumeWriteLock(); // see if we are re-attaching and previously had the write lock. - if (!m_writeLockHeld && HasLocalChanges()) + BeJsDocument lockedBy; + ReadWriteLock(lockedBy); + auto lockedByGuid = lockedBy[JSON_NAME(guid)].asString(); + if ((!lockedByGuid.Equals(m_cache->m_guid) || !m_writeLockHeld) && HasLocalChanges()) AbandonChanges(info); // we lost the write lock, we have no choice but to abandon all local changes. } From 7519d5fe70a2fd831236b260c378432f2f577093 Mon Sep 17 00:00:00 2001 From: "nick.tessier" <22119573+nick4598@users.noreply.github.com> Date: Tue, 1 Oct 2024 14:13:21 -0400 Subject: [PATCH 07/12] maybe different approach to Connect and resuming write lock? ResumeWriteLock() now sets m_writeLockHeld to false. --- iModelJsNodeAddon/JsCloudSqlite.cpp | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/iModelJsNodeAddon/JsCloudSqlite.cpp b/iModelJsNodeAddon/JsCloudSqlite.cpp index 8c635f6ba1b..804b428b12c 100644 --- a/iModelJsNodeAddon/JsCloudSqlite.cpp +++ b/iModelJsNodeAddon/JsCloudSqlite.cpp @@ -234,10 +234,7 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { if (m_writeable) { ResumeWriteLock(); // see if we are re-attaching and previously had the write lock. - BeJsDocument lockedBy; - ReadWriteLock(lockedBy); - auto lockedByGuid = lockedBy[JSON_NAME(guid)].asString(); - if ((!lockedByGuid.Equals(m_cache->m_guid) || !m_writeLockHeld) && HasLocalChanges()) + if ((!m_writeLockHeld && HasLocalChanges())) AbandonChanges(info); // we lost the write lock, we have no choice but to abandon all local changes. } @@ -562,6 +559,7 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { BeJsDocument lockedBy; ReadWriteLock(lockedBy); + m_writeLockHeld = false; auto lockedByGuid = lockedBy[JSON_NAME(guid)].asString(); if (lockedByGuid.Equals(m_cache->m_guid)) { try { From d2e396de7dcd4c71d6eb7f5b66f46d44a1d7d236 Mon Sep 17 00:00:00 2001 From: "nick.tessier" <22119573+nick4598@users.noreply.github.com> Date: Tue, 1 Oct 2024 14:13:54 -0400 Subject: [PATCH 08/12] paren --- iModelJsNodeAddon/JsCloudSqlite.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/iModelJsNodeAddon/JsCloudSqlite.cpp b/iModelJsNodeAddon/JsCloudSqlite.cpp index 804b428b12c..207b3b3064e 100644 --- a/iModelJsNodeAddon/JsCloudSqlite.cpp +++ b/iModelJsNodeAddon/JsCloudSqlite.cpp @@ -234,7 +234,7 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { if (m_writeable) { ResumeWriteLock(); // see if we are re-attaching and previously had the write lock. - if ((!m_writeLockHeld && HasLocalChanges())) + if (!m_writeLockHeld && HasLocalChanges()) AbandonChanges(info); // we lost the write lock, we have no choice but to abandon all local changes. } From 4a1b0ca587d978849d4614cb80d34c90a5442d9e Mon Sep 17 00:00:00 2001 From: "nick.tessier" <22119573+nick4598@users.noreply.github.com> Date: Wed, 2 Oct 2024 10:31:46 -0400 Subject: [PATCH 09/12] check if its not locked at all and throw error if so --- iModelJsNodeAddon/JsCloudSqlite.cpp | 27 +++++++++++++++++++-------- 1 file changed, 19 insertions(+), 8 deletions(-) diff --git a/iModelJsNodeAddon/JsCloudSqlite.cpp b/iModelJsNodeAddon/JsCloudSqlite.cpp index 207b3b3064e..a0c24868a15 100644 --- a/iModelJsNodeAddon/JsCloudSqlite.cpp +++ b/iModelJsNodeAddon/JsCloudSqlite.cpp @@ -193,17 +193,16 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { RequireConnected(); BeJsDocument lockedBy; ReadWriteLock(lockedBy); - auto lockedByGuid = lockedBy[JSON_NAME(guid)].asString(); - // check if it's the same guid - if (!lockedByGuid.Equals(m_cache->m_guid)) { + auto lockedByGuid = lockedBy[JSON_NAME(guid)].asString(""); + + if (lockedByGuid.Equals("")) { + BeNapi::ThrowJsException(Env(), Utf8PrintfString("Container [%s] is not locked for write access.", m_containerId.c_str()).c_str()); + } else if (!lockedByGuid.Equals(m_cache->m_guid)) { // another user grabbed the write lock after the current user's write lock expiration time, disable current user from operating BeNapi::ThrowJsException(Env(), Utf8PrintfString("Container [%s] is currently locked by another user.", m_containerId.c_str()).c_str()); } else { - // Refresh the write lock for the current user no matter if the write lock expires - // 1. If the write lock expires, refresh it for the current user to operate further actions - // 2. If not, also refresh it in case the current user's write lock is about to expire when they call operations that require write lock to be present, and somehow expires during the operations + // Refresh the write lock for the current user incase their write lock is about to expire. ResumeWriteLock(); - } } @@ -555,12 +554,24 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { throw err; } - void ResumeWriteLock() { + void ResumeWriteLock(bool shouldFail) { BeJsDocument lockedBy; ReadWriteLock(lockedBy); m_writeLockHeld = false; auto lockedByGuid = lockedBy[JSON_NAME(guid)].asString(); + + // Why can't I just call AcquireWriteLock here? do I really need to read the lock? + // I guess one benefit of doing the excessive read is that we could for some unrelated reason fail to acquireWriteLock + // but still have the lock? Idk how likely that is + // I guess resumeWriteLock in one case (on the connect, should definitely not fail. But in the other case, it probably should fail) + // That would cut out 1 read lock. In order to cut out 1 more would ahve to remove the error handling from requriewritelock. + // The thing is, I want people to be explicit about grabbing the write lock. I don't want to just grab it for them. + // So requireWriteLock should fail if they don't have it.. I think that sort've requires an extra read.. I mean it doesnt.. + // but its more changes required. I would have to pass to AcquireWriteLock a bool that says to fail if the lock is not held already. + // + + if (lockedByGuid.Equals(m_cache->m_guid)) { try { AcquireWriteLock(lockedBy[JSON_NAME(user)].asString()); From 3de8fcb5bce8b7a46b56f2c92ee5e301e5e0926d Mon Sep 17 00:00:00 2001 From: "nick.tessier" <22119573+nick4598@users.noreply.github.com> Date: Wed, 9 Oct 2024 11:48:15 -0400 Subject: [PATCH 10/12] reduce amount of consecutive bcv_kv reads --- iModelJsNodeAddon/JsCloudSqlite.cpp | 88 +++++++++++++++++------------ 1 file changed, 51 insertions(+), 37 deletions(-) diff --git a/iModelJsNodeAddon/JsCloudSqlite.cpp b/iModelJsNodeAddon/JsCloudSqlite.cpp index a0c24868a15..382f5754dad 100644 --- a/iModelJsNodeAddon/JsCloudSqlite.cpp +++ b/iModelJsNodeAddon/JsCloudSqlite.cpp @@ -191,19 +191,20 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { void RequireWriteLock() { RequireConnected(); - BeJsDocument lockedBy; - ReadWriteLock(lockedBy); - auto lockedByGuid = lockedBy[JSON_NAME(guid)].asString(""); - - if (lockedByGuid.Equals("")) { - BeNapi::ThrowJsException(Env(), Utf8PrintfString("Container [%s] is not locked for write access.", m_containerId.c_str()).c_str()); - } else if (!lockedByGuid.Equals(m_cache->m_guid)) { - // another user grabbed the write lock after the current user's write lock expiration time, disable current user from operating - BeNapi::ThrowJsException(Env(), Utf8PrintfString("Container [%s] is currently locked by another user.", m_containerId.c_str()).c_str()); - } else { - // Refresh the write lock for the current user incase their write lock is about to expire. - ResumeWriteLock(); - } + ResumeWriteLock(true); + // BeJsDocument lockedBy; + // ReadWriteLock(lockedBy); + // auto lockedByGuid = lockedBy[JSON_NAME(guid)].asString(""); + + // if (lockedByGuid.Equals("")) { + // BeNapi::ThrowJsException(Env(), Utf8PrintfString("Container [%s] is not locked for write access.", m_containerId.c_str()).c_str()); + // } else if (!lockedByGuid.Equals(m_cache->m_guid)) { + // // another user grabbed the write lock after the current user's write lock expiration time, disable current user from operating + // BeNapi::ThrowJsException(Env(), Utf8PrintfString("Container [%s] is currently locked by another user.", m_containerId.c_str()).c_str()); + // } else { + // // Refresh the write lock for the current user incase their write lock is about to expire. + // ResumeWriteLock(); + // } } void CallJsMemberFunc(Utf8CP funcName, std::vector const& args) { @@ -232,7 +233,7 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { m_writeable = false; // daemon cannot be used for writing if (m_writeable) { - ResumeWriteLock(); // see if we are re-attaching and previously had the write lock. + ResumeWriteLock(false); // see if we are re-attaching and previously had the write lock. if (!m_writeLockHeld && HasLocalChanges()) AbandonChanges(info); // we lost the write lock, we have no choice but to abandon all local changes. } @@ -516,7 +517,7 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { rc = stmt.Step(); if (rc == BE_SQLITE_ROW) { auto date = DateTime::FromString(stmt.GetValueText(0)); - if (date.IsValid()) + if (date.IsValid()) return date; } // if the server didn't include a "Date" field, just fall back to local time. @@ -525,28 +526,31 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { return DateTime::GetCurrentTimeUtc(); } - void CheckLock() { + Utf8String CheckLock(bool failIfNotHeldAlready) { BeJsDocument lockedBy; ReadWriteLock(lockedBy); auto lockedByGuid = lockedBy[JSON_NAME(guid)].asString(); - if (lockedByGuid.empty() || lockedByGuid.Equals(m_cache->m_guid)) - return; // not locked or already locked by same cache + Utf8String lockedByUser = lockedBy[JSON_NAME(user)].asString(); + if (failIfNotHeldAlready && lockedByGuid.empty()) { // TODO: can i simplify? + m_containerDb.TryExecuteSql("ROLLBACK"); + BeNapi::ThrowJsException(Env(), Utf8PrintfString("Container [%s] is not locked for write access.", m_containerId.c_str()).c_str()); + } else if (lockedByGuid.empty() || lockedByGuid.Equals(m_cache->m_guid)) + return lockedByUser; // not locked or already locked by same cache ( we should use the same user when possible I guess??) auto expiresAt = DateTime::FromString(lockedBy[JSON_NAME(expires)].asString()); if (!expiresAt.IsValid()) - return; // the expiration time is invalid, ignore lock + return lockedByUser; // the expiration time is invalid, ignore lock - auto lockedByUser = lockedBy[JSON_NAME(user)].asString(); if (DateTime::CompareResult::EarlierThan == DateTime::Compare(expiresAt, GetServerTime())) { Utf8PrintfString warning("write lock on container [%s] from user [%s] was present but expired. Overwriting it.", m_containerId.c_str(), lockedByUser.c_str()); NativeLogging::Logging::LogMessage("CloudSqlite", NativeLogging::SEVERITY::LOG_WARNING, warning.c_str()); - return; // other user's write lock has expired + return lockedByUser; // other user's write lock has expired } m_containerDb.TryExecuteSql("ROLLBACK"); // report that container is currently locked by another user. Set details in exception. - auto err = Napi::Error::New(Env(), Utf8PrintfString("Container [%s] is currently locked.", m_containerId.c_str()).c_str()); + auto err = Napi::Error::New(Env(), Utf8PrintfString("Container [%s] is currently locked by another user.", m_containerId.c_str()).c_str()); auto val = err.Value(); val[JSON_NAME(errorNumber)] = (int) BE_SQLITE_BUSY; val[JSON_NAME(lockedBy)] = lockedByUser.c_str(); @@ -554,12 +558,12 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { throw err; } - void ResumeWriteLock(bool shouldFail) { - BeJsDocument lockedBy; - ReadWriteLock(lockedBy); + void ResumeWriteLock(bool shouldFailIfUnableToResume) { + // BeJsDocument lockedBy; + // ReadWriteLock(lockedBy); m_writeLockHeld = false; - auto lockedByGuid = lockedBy[JSON_NAME(guid)].asString(); + // auto lockedByGuid = lockedBy[JSON_NAME(guid)].asString(); // Why can't I just call AcquireWriteLock here? do I really need to read the lock? // I guess one benefit of doing the excessive read is that we could for some unrelated reason fail to acquireWriteLock @@ -570,15 +574,24 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { // So requireWriteLock should fail if they don't have it.. I think that sort've requires an extra read.. I mean it doesnt.. // but its more changes required. I would have to pass to AcquireWriteLock a bool that says to fail if the lock is not held already. // - - - if (lockedByGuid.Equals(m_cache->m_guid)) { + if (shouldFailIfUnableToResume) { + // Shit I need the username!! I guess we would need CheckLock to return the username. This might ruin everything. + AcquireWriteLock("", true); + } else { try { - AcquireWriteLock(lockedBy[JSON_NAME(user)].asString()); + AcquireWriteLock("", true); // TODO? } catch (...) { - // if we can't resume, don't fail + // if we can't resume, don't fail. } } + + // if (lockedByGuid.Equals(m_cache->m_guid)) { + // try { + // AcquireWriteLock(lockedBy[JSON_NAME(user)].asString()); + // } catch (...) { + // // if we can't resume, don't fail + // } + // } } Utf8String GetServerDateString(uint64_t offsetMilliseconds) { @@ -587,18 +600,18 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { return DateTime::FromJulianDay(serverTimeMs + offsetMilliseconds, DateTime::Info::CreateForDateTime(DateTime::Kind::Utc)).ToString(); } - void AcquireWriteLock(Utf8StringCR user) { + void AcquireWriteLock(Utf8StringCR user, bool failIfNotHeldAlready) { RequireConnected(); if (!m_writeable) BeNapi::ThrowJsException(Env(), "container is not writeable"); m_containerDb.TryExecuteSql("BEGIN"); - CheckLock(); // throws if already locked by another user + Utf8String lockOwner = CheckLock(failIfNotHeldAlready); // throws if already locked by another user or failIfNotHeldAlready is true and not locked at all - m_lockExpireSeconds = std::min((int) (12*SECONDS_PER_HOUR), std::max(5*((int)SECONDS_PER_MINUTE), m_lockExpireSeconds)); + m_lockExpireSeconds = std::min((int) (12*SECONDS_PER_HOUR), std::max(10*((int)SECONDS_PER_MINUTE), m_lockExpireSeconds)); BeJsDocument lockedBy; lockedBy[JSON_NAME(guid)] = m_cache->m_guid; - lockedBy[JSON_NAME(user)] = user; + lockedBy[JSON_NAME(user)] = user.Equals("") ? lockOwner : user; lockedBy[JSON_NAME(expires)] = GetServerDateString(m_lockExpireSeconds * 1000); Statement stmt; @@ -619,7 +632,7 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { void AcquireWriteLockJs(NapiInfoCR info) { REQUIRE_ARGUMENT_STRING(0, user); - AcquireWriteLock(user); + AcquireWriteLock(user, false); PollManifest(info); } @@ -666,6 +679,7 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { void ReleaseWriteLock(NapiInfoCR info) { RequireConnected(); + ResumeWriteLock(false); // We can only trust the truth value of m_writeLockHeld after calling ResumeWriteLock if (!m_writeLockHeld) return; @@ -679,7 +693,7 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { void AbandonChanges(NapiInfoCR info) { RequireConnected(); - + ResumeWriteLock(false); // We can only trust the truth value of m_writeLockHeld after calling ResumeWriteLock CloudResult stat; if (m_writeLockHeld) stat = ClearWriteLock(); From 9c5a4964ccd5400efd3fe04728a014f21bde4610 Mon Sep 17 00:00:00 2001 From: "nick.tessier" <22119573+nick4598@users.noreply.github.com> Date: Wed, 9 Oct 2024 16:34:02 -0400 Subject: [PATCH 11/12] remove scratch work --- iModelJsNodeAddon/JsCloudSqlite.cpp | 40 ++--------------------------- 1 file changed, 2 insertions(+), 38 deletions(-) diff --git a/iModelJsNodeAddon/JsCloudSqlite.cpp b/iModelJsNodeAddon/JsCloudSqlite.cpp index 382f5754dad..d75c030a080 100644 --- a/iModelJsNodeAddon/JsCloudSqlite.cpp +++ b/iModelJsNodeAddon/JsCloudSqlite.cpp @@ -192,19 +192,6 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { void RequireWriteLock() { RequireConnected(); ResumeWriteLock(true); - // BeJsDocument lockedBy; - // ReadWriteLock(lockedBy); - // auto lockedByGuid = lockedBy[JSON_NAME(guid)].asString(""); - - // if (lockedByGuid.Equals("")) { - // BeNapi::ThrowJsException(Env(), Utf8PrintfString("Container [%s] is not locked for write access.", m_containerId.c_str()).c_str()); - // } else if (!lockedByGuid.Equals(m_cache->m_guid)) { - // // another user grabbed the write lock after the current user's write lock expiration time, disable current user from operating - // BeNapi::ThrowJsException(Env(), Utf8PrintfString("Container [%s] is currently locked by another user.", m_containerId.c_str()).c_str()); - // } else { - // // Refresh the write lock for the current user incase their write lock is about to expire. - // ResumeWriteLock(); - // } } void CallJsMemberFunc(Utf8CP funcName, std::vector const& args) { @@ -535,7 +522,7 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { m_containerDb.TryExecuteSql("ROLLBACK"); BeNapi::ThrowJsException(Env(), Utf8PrintfString("Container [%s] is not locked for write access.", m_containerId.c_str()).c_str()); } else if (lockedByGuid.empty() || lockedByGuid.Equals(m_cache->m_guid)) - return lockedByUser; // not locked or already locked by same cache ( we should use the same user when possible I guess??) + return lockedByUser; // not locked or already locked by same cache, we'll use the same user. auto expiresAt = DateTime::FromString(lockedBy[JSON_NAME(expires)].asString()); if (!expiresAt.IsValid()) @@ -559,39 +546,16 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { } void ResumeWriteLock(bool shouldFailIfUnableToResume) { - // BeJsDocument lockedBy; - // ReadWriteLock(lockedBy); - m_writeLockHeld = false; - // auto lockedByGuid = lockedBy[JSON_NAME(guid)].asString(); - - // Why can't I just call AcquireWriteLock here? do I really need to read the lock? - // I guess one benefit of doing the excessive read is that we could for some unrelated reason fail to acquireWriteLock - // but still have the lock? Idk how likely that is - // I guess resumeWriteLock in one case (on the connect, should definitely not fail. But in the other case, it probably should fail) - // That would cut out 1 read lock. In order to cut out 1 more would ahve to remove the error handling from requriewritelock. - // The thing is, I want people to be explicit about grabbing the write lock. I don't want to just grab it for them. - // So requireWriteLock should fail if they don't have it.. I think that sort've requires an extra read.. I mean it doesnt.. - // but its more changes required. I would have to pass to AcquireWriteLock a bool that says to fail if the lock is not held already. - // if (shouldFailIfUnableToResume) { - // Shit I need the username!! I guess we would need CheckLock to return the username. This might ruin everything. AcquireWriteLock("", true); } else { try { - AcquireWriteLock("", true); // TODO? + AcquireWriteLock("", true); } catch (...) { // if we can't resume, don't fail. } } - - // if (lockedByGuid.Equals(m_cache->m_guid)) { - // try { - // AcquireWriteLock(lockedBy[JSON_NAME(user)].asString()); - // } catch (...) { - // // if we can't resume, don't fail - // } - // } } Utf8String GetServerDateString(uint64_t offsetMilliseconds) { From ce71ee738f461104d1b89fb94332afd6d7876910 Mon Sep 17 00:00:00 2001 From: "nick.tessier" <22119573+nick4598@users.noreply.github.com> Date: Fri, 11 Oct 2024 11:28:14 -0400 Subject: [PATCH 12/12] remove todo --- iModelJsNodeAddon/JsCloudSqlite.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/iModelJsNodeAddon/JsCloudSqlite.cpp b/iModelJsNodeAddon/JsCloudSqlite.cpp index d75c030a080..17a9af78cb3 100644 --- a/iModelJsNodeAddon/JsCloudSqlite.cpp +++ b/iModelJsNodeAddon/JsCloudSqlite.cpp @@ -518,7 +518,7 @@ struct JsCloudContainer : CloudContainer, Napi::ObjectWrap { ReadWriteLock(lockedBy); auto lockedByGuid = lockedBy[JSON_NAME(guid)].asString(); Utf8String lockedByUser = lockedBy[JSON_NAME(user)].asString(); - if (failIfNotHeldAlready && lockedByGuid.empty()) { // TODO: can i simplify? + if (failIfNotHeldAlready && lockedByGuid.empty()) { m_containerDb.TryExecuteSql("ROLLBACK"); BeNapi::ThrowJsException(Env(), Utf8PrintfString("Container [%s] is not locked for write access.", m_containerId.c_str()).c_str()); } else if (lockedByGuid.empty() || lockedByGuid.Equals(m_cache->m_guid))