From 4103f8569d1068802ad587cf8d281f2384503cab Mon Sep 17 00:00:00 2001 From: Alexey Orlenko Date: Tue, 21 Nov 2023 19:05:02 +0100 Subject: [PATCH 1/9] driver-adapters: remove dispose method for JS transaction interface Remove the boilerplate `Transaction.dispose` method from the public driver adapter interface and move the corresponding functionality directly to the destructor of `TransactionProxy`. Refs: https://github.com/prisma/prisma-engines/pull/4286 Closes: https://github.com/prisma/team-orm/issues/391 --- .../driver-adapters/src/async_js_function.rs | 4 +++ query-engine/driver-adapters/src/proxy.rs | 33 ++++++++++++++----- 2 files changed, 28 insertions(+), 9 deletions(-) diff --git a/query-engine/driver-adapters/src/async_js_function.rs b/query-engine/driver-adapters/src/async_js_function.rs index 5f535334ffb9..4926534f58b1 100644 --- a/query-engine/driver-adapters/src/async_js_function.rs +++ b/query-engine/driver-adapters/src/async_js_function.rs @@ -55,6 +55,10 @@ where .map_err(into_quaint_error)?; js_result.into() } + + pub(crate) fn as_raw(&self) -> &ThreadsafeFunction { + &self.threadsafe_fn + } } impl FromNapiValue for AsyncJsFunction diff --git a/query-engine/driver-adapters/src/proxy.rs b/query-engine/driver-adapters/src/proxy.rs index 642c2491757a..000203c0d228 100644 --- a/query-engine/driver-adapters/src/proxy.rs +++ b/query-engine/driver-adapters/src/proxy.rs @@ -1,12 +1,12 @@ use std::borrow::Cow; use std::str::FromStr; +use std::sync::atomic::{AtomicBool, Ordering}; use crate::async_js_function::AsyncJsFunction; use crate::conversion::JSArg; use crate::transaction::JsTransaction; use metrics::increment_gauge; use napi::bindgen_prelude::{FromNapiValue, ToNapiValue}; -use napi::threadsafe_function::{ErrorStrategy, ThreadsafeFunction}; use napi::{JsObject, JsString}; use napi_derive::napi; use quaint::connector::ResultSet as QuaintResultSet; @@ -52,9 +52,8 @@ pub(crate) struct TransactionProxy { /// rollback transaction rollback: AsyncJsFunction<(), ()>, - /// dispose transaction, cleanup logic executed at the end of the transaction lifecycle - /// on drop. - dispose: ThreadsafeFunction<(), ErrorStrategy::Fatal>, + /// whether the transaction has already been committed or rolled back + closed: AtomicBool, } /// This result set is more convenient to be manipulated from both Rust and NodeJS. @@ -581,14 +580,13 @@ impl TransactionProxy { pub fn new(js_transaction: &JsObject) -> napi::Result { let commit = js_transaction.get_named_property("commit")?; let rollback = js_transaction.get_named_property("rollback")?; - let dispose = js_transaction.get_named_property("dispose")?; let options = js_transaction.get_named_property("options")?; Ok(Self { commit, rollback, - dispose, options, + closed: AtomicBool::new(false), }) } @@ -596,19 +594,36 @@ impl TransactionProxy { &self.options } + /// Commits the transaction via the driver adapter. + /// + /// Cancelation safety: [`TransactionProxy::closed`] is only set after the call to JS finishes, + /// so the destructor will ensure the transaction is closed even if the future is dropped. pub async fn commit(&self) -> quaint::Result<()> { - self.commit.call(()).await + let result = self.commit.call(()).await; + self.closed.swap(true, Ordering::Release); + result } + /// Rolls back the transaction via the driver adapter. + /// + /// Cancelation safety: [`TransactionProxy::closed`] is only set after the call to JS finishes, + /// so the destructor will ensure the transaction is closed even if the future is dropped. pub async fn rollback(&self) -> quaint::Result<()> { - self.rollback.call(()).await + let result = self.rollback.call(()).await; + self.closed.swap(true, Ordering::Release); + result } } impl Drop for TransactionProxy { fn drop(&mut self) { + if self.closed.swap(true, Ordering::Acquire) { + return; + } + _ = self - .dispose + .rollback + .as_raw() .call((), napi::threadsafe_function::ThreadsafeFunctionCallMode::NonBlocking); } } From 71e54b475fcfeb623f583de201a66faa1c0895f0 Mon Sep 17 00:00:00 2001 From: Alexey Orlenko Date: Tue, 21 Nov 2023 21:32:18 +0100 Subject: [PATCH 2/9] Use store instead of swap --- query-engine/driver-adapters/src/proxy.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/query-engine/driver-adapters/src/proxy.rs b/query-engine/driver-adapters/src/proxy.rs index 000203c0d228..c3d5a3eaf037 100644 --- a/query-engine/driver-adapters/src/proxy.rs +++ b/query-engine/driver-adapters/src/proxy.rs @@ -600,7 +600,7 @@ impl TransactionProxy { /// so the destructor will ensure the transaction is closed even if the future is dropped. pub async fn commit(&self) -> quaint::Result<()> { let result = self.commit.call(()).await; - self.closed.swap(true, Ordering::Release); + self.closed.store(true, Ordering::Release); result } @@ -610,7 +610,7 @@ impl TransactionProxy { /// so the destructor will ensure the transaction is closed even if the future is dropped. pub async fn rollback(&self) -> quaint::Result<()> { let result = self.rollback.call(()).await; - self.closed.swap(true, Ordering::Release); + self.closed.store(true, Ordering::Release); result } } From 6cde7b17d8706b7c519369049886597937c5a391 Mon Sep 17 00:00:00 2001 From: Alexey Orlenko Date: Wed, 22 Nov 2023 14:51:37 +0100 Subject: [PATCH 3/9] Add test --- .../tests/new/regressions/mod.rs | 1 + .../new/regressions/prisma_engines_4286.rs | 24 +++++++++++++++++++ 2 files changed, 25 insertions(+) create mode 100644 query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_engines_4286.rs diff --git a/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/mod.rs b/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/mod.rs index 0714015efd06..252ba620b5ff 100644 --- a/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/mod.rs +++ b/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/mod.rs @@ -27,3 +27,4 @@ mod prisma_7010; mod prisma_7072; mod prisma_7434; mod prisma_8265; +mod prisma_engines_4286; diff --git a/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_engines_4286.rs b/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_engines_4286.rs new file mode 100644 index 000000000000..d6428d752359 --- /dev/null +++ b/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_engines_4286.rs @@ -0,0 +1,24 @@ +use query_engine_tests::*; + +#[test_suite(schema(generic), only(Sqlite))] +mod sqlite { + #[connector_test] + async fn close_tx_on_error(runner: Runner) -> TestResult<()> { + // Try to open a transaction with unsupported isolation error in SQLite. + let result = runner.start_tx(2000, 5000, Some("ReadUncommitted".to_owned())).await; + assert!(result.is_err()); + + // Without the changes from https://github.com/prisma/prisma-engines/pull/4286 or + // https://github.com/prisma/prisma-engines/pull/4489 this second `start_tx` call will + // either hang infinitely with libSQL driver adapter, or fail with a "cannot start a + // transaction within a transaction" error. + // A more future proof way to check this would be to make both transactions EXCLUSIVE or + // IMMEDIATE if we had control over SQLite transaction type here, as that would not rely on + // both transactions using the same connection if we were to pool multiple SQLite + // connections in the future. + let tx = runner.start_tx(2000, 5000, None).await?; + runner.rollback_tx(tx).await?.unwrap(); + + Ok(()) + } +} From 5f72dde7fb2d7dafc7032fd241c1d440c8da068a Mon Sep 17 00:00:00 2001 From: Alexey Orlenko Date: Wed, 22 Nov 2023 15:40:58 +0100 Subject: [PATCH 4/9] Only run test with driver adapters --- .../tests/new/regressions/prisma_engines_4286.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_engines_4286.rs b/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_engines_4286.rs index d6428d752359..0e5410c0c5b6 100644 --- a/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_engines_4286.rs +++ b/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_engines_4286.rs @@ -1,6 +1,6 @@ use query_engine_tests::*; -#[test_suite(schema(generic), only(Sqlite))] +#[test_suite(schema(generic), only(JS, Sqlite))] mod sqlite { #[connector_test] async fn close_tx_on_error(runner: Runner) -> TestResult<()> { From a66a01954780b5169b7991a98444d5c121f17025 Mon Sep 17 00:00:00 2001 From: Alexey Orlenko Date: Thu, 23 Nov 2023 14:15:36 +0100 Subject: [PATCH 5/9] Change order of operations in commit and rollback --- query-engine/driver-adapters/src/proxy.rs | 36 ++++++++++++++++++----- 1 file changed, 28 insertions(+), 8 deletions(-) diff --git a/query-engine/driver-adapters/src/proxy.rs b/query-engine/driver-adapters/src/proxy.rs index c3d5a3eaf037..694d7368a4c0 100644 --- a/query-engine/driver-adapters/src/proxy.rs +++ b/query-engine/driver-adapters/src/proxy.rs @@ -596,22 +596,42 @@ impl TransactionProxy { /// Commits the transaction via the driver adapter. /// - /// Cancelation safety: [`TransactionProxy::closed`] is only set after the call to JS finishes, - /// so the destructor will ensure the transaction is closed even if the future is dropped. + /// ## Cancellation safety + /// + /// The future is cancellation-safe as long as the underlying Node-API call + /// is cancellation-safe and no new await points are introduced between storing true in + /// [`TransactionProxy::closed`] and calling the underlying JS function. + /// + /// - If `commit` is called but never polled or awaited, it's a no-op, the transaction won't be + /// committed and [`TransactionProxy::closed`] will not be changed. + /// + /// - If it is polled at least once, `true` will be stored in [`TransactionProxy::closed`] and + /// the underlying FFI call will be delivered to JavaScript side in lockstep, so the destructor + /// will not attempt rolling the transaction back even if the `commit` future was dropped while + /// waiting on the JavaScript call. pub async fn commit(&self) -> quaint::Result<()> { - let result = self.commit.call(()).await; self.closed.store(true, Ordering::Release); - result + self.commit.call(()).await } /// Rolls back the transaction via the driver adapter. /// - /// Cancelation safety: [`TransactionProxy::closed`] is only set after the call to JS finishes, - /// so the destructor will ensure the transaction is closed even if the future is dropped. + /// ## Cancellation safety + /// + /// The future is cancellation-safe as long as the underlying Node-API call + /// is cancellation-safe and no new await points are introduced between storing true in + /// [`TransactionProxy::closed`] and calling the underlying JS function. + /// + /// - If `rollback` is called but never polled or awaited, it's a no-op, the transaction won't be + /// rolled back yet and [`TransactionProxy::closed`] will not be changed. + /// + /// - If it is polled at least once, `true` will be stored in [`TransactionProxy::closed`] and + /// the underlying FFI call will be delivered to JavaScript side in lockstep, so the destructor + /// will not attempt rolling back again even if the `rollback` future was dropped while waiting + /// on the JavaScript call. pub async fn rollback(&self) -> quaint::Result<()> { - let result = self.rollback.call(()).await; self.closed.store(true, Ordering::Release); - result + self.rollback.call(()).await } } From 5ab9f4f1eb559575c1ef7ed6cdfd722de5620327 Mon Sep 17 00:00:00 2001 From: Alexey Orlenko Date: Thu, 23 Nov 2023 14:17:50 +0100 Subject: [PATCH 6/9] Clarify wording a bit --- query-engine/driver-adapters/src/proxy.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/query-engine/driver-adapters/src/proxy.rs b/query-engine/driver-adapters/src/proxy.rs index 694d7368a4c0..c065c8eb0487 100644 --- a/query-engine/driver-adapters/src/proxy.rs +++ b/query-engine/driver-adapters/src/proxy.rs @@ -608,7 +608,7 @@ impl TransactionProxy { /// - If it is polled at least once, `true` will be stored in [`TransactionProxy::closed`] and /// the underlying FFI call will be delivered to JavaScript side in lockstep, so the destructor /// will not attempt rolling the transaction back even if the `commit` future was dropped while - /// waiting on the JavaScript call. + /// waiting on the JavaScript call to complete and deliver response. pub async fn commit(&self) -> quaint::Result<()> { self.closed.store(true, Ordering::Release); self.commit.call(()).await @@ -628,7 +628,7 @@ impl TransactionProxy { /// - If it is polled at least once, `true` will be stored in [`TransactionProxy::closed`] and /// the underlying FFI call will be delivered to JavaScript side in lockstep, so the destructor /// will not attempt rolling back again even if the `rollback` future was dropped while waiting - /// on the JavaScript call. + /// on the JavaScript call to complete and deliver response. pub async fn rollback(&self) -> quaint::Result<()> { self.closed.store(true, Ordering::Release); self.rollback.call(()).await From 394895626de6157e9d27411586897a0537737aec Mon Sep 17 00:00:00 2001 From: Alexey Orlenko Date: Fri, 24 Nov 2023 15:54:37 +0100 Subject: [PATCH 7/9] Change `only` tag from generic `JS` to `libsql.js` MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Miguel Fernández --- .../tests/new/regressions/prisma_engines_4286.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_engines_4286.rs b/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_engines_4286.rs index 0e5410c0c5b6..3bb4e0a35637 100644 --- a/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_engines_4286.rs +++ b/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_engines_4286.rs @@ -1,6 +1,6 @@ use query_engine_tests::*; -#[test_suite(schema(generic), only(JS, Sqlite))] +#[test_suite(schema(generic), only(Sqlite('libsql.js')))] mod sqlite { #[connector_test] async fn close_tx_on_error(runner: Runner) -> TestResult<()> { From 96e9ec898030c0e39b005863dfad1e4892a1e5f0 Mon Sep 17 00:00:00 2001 From: Alexey Orlenko Date: Fri, 24 Nov 2023 22:11:30 +0100 Subject: [PATCH 8/9] Relax ordering mode of `TransactionProxy::closed` modifications There's no need in release-store in commit/rollback and acquire-load in destructor anymore since we don't care for commit to have finished in the destructor anymore. --- query-engine/driver-adapters/src/proxy.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/query-engine/driver-adapters/src/proxy.rs b/query-engine/driver-adapters/src/proxy.rs index c065c8eb0487..19693453988e 100644 --- a/query-engine/driver-adapters/src/proxy.rs +++ b/query-engine/driver-adapters/src/proxy.rs @@ -610,7 +610,7 @@ impl TransactionProxy { /// will not attempt rolling the transaction back even if the `commit` future was dropped while /// waiting on the JavaScript call to complete and deliver response. pub async fn commit(&self) -> quaint::Result<()> { - self.closed.store(true, Ordering::Release); + self.closed.store(true, Ordering::Relaxed); self.commit.call(()).await } @@ -630,14 +630,14 @@ impl TransactionProxy { /// will not attempt rolling back again even if the `rollback` future was dropped while waiting /// on the JavaScript call to complete and deliver response. pub async fn rollback(&self) -> quaint::Result<()> { - self.closed.store(true, Ordering::Release); + self.closed.store(true, Ordering::Relaxed); self.rollback.call(()).await } } impl Drop for TransactionProxy { fn drop(&mut self) { - if self.closed.swap(true, Ordering::Acquire) { + if self.closed.swap(true, Ordering::Relaxed) { return; } From 8e0f716249c1d4f0d34a51b38fd8bc6cea50cc3d Mon Sep 17 00:00:00 2001 From: Alexey Orlenko Date: Sat, 25 Nov 2023 01:07:18 +0100 Subject: [PATCH 9/9] Fix the exclude tag --- .../tests/new/regressions/prisma_engines_4286.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_engines_4286.rs b/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_engines_4286.rs index 3bb4e0a35637..313a29cdacf4 100644 --- a/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_engines_4286.rs +++ b/query-engine/connector-test-kit-rs/query-engine-tests/tests/new/regressions/prisma_engines_4286.rs @@ -1,6 +1,6 @@ use query_engine_tests::*; -#[test_suite(schema(generic), only(Sqlite('libsql.js')))] +#[test_suite(schema(generic), only(Sqlite("libsql.js")))] mod sqlite { #[connector_test] async fn close_tx_on_error(runner: Runner) -> TestResult<()> {