driver-adapters: close tx on drop #4286
Conversation
CodSpeed Performance ReportMerging #4286 will not alter performanceComparing Summary
|
| @@ -96,6 +96,13 @@ export interface Transaction extends Queryable { | |||
| * Rolls back the transaction. | |||
| */ | |||
| rollback(): Promise<Result<void>> | |||
| /** | |||
| * Discards and closes the transaction which may or may not have been committed or rolled back. | |||
| * This operation must be synchronous. If the implementation requires calling creating new | |||
There was a problem hiding this comment.
Not sure if requiring a synchronous function is the best approach, perhaps it would be better to let it be async, and either add a method to AsyncJsFunction that allows calling it in the background ignoring the result, or spawn a new tokio task in JsTransaction::drop that will actually await it and maybe log an error if it happens.
There was a problem hiding this comment.
Close trait's close is not async, so we might want to keep this synchronous for convenience, unless we measure that it can compromise throughput.
There was a problem hiding this comment.
It's not even about throughput, we'll have to do exactly the same amount of work synchronously until the first yield point in async function or end of sync function regardless of what we choose. The implementation of this method for Turso and PlanetScale is only "synchronous" in that it doesn't return a Promise and spawns a task on the event loop without waiting for it, and the implementation for pg and Neon would still only use synchronous operations even if the method was marked as async.
So changing this to be async should not really affect the performance or improve throughput but might arguably be a little cleaner, and might be a little more convenient when implementing new driver adapters (it will be harder to accidentally end up with unhandled promise rejections there).
Not something that we must decide right now, of course, we can improve this later.
There was a problem hiding this comment.
Thanks for the explanation. makes sense.
| @@ -96,6 +96,13 @@ export interface Transaction extends Queryable { | |||
| * Rolls back the transaction. | |||
| */ | |||
| rollback(): Promise<Result<void>> | |||
| /** | |||
| * Discards and closes the transaction which may or may not have been committed or rolled back. | |||
| * This operation must be synchronous. If the implementation requires calling creating new | |||
There was a problem hiding this comment.
Close trait's close is not async, so we might want to keep this synchronous for convenience, unless we measure that it can compromise throughput.
Remove the boilerplate `Transaction.dispose` method from the public driver adapter interface and move the corresponding functionality directly to the destructor of `TransactionProxy`. Refs: #4286 Closes: prisma/team-orm#391
Remove the boilerplate `Transaction.dispose` method from the public driver adapter interface and move the corresponding functionality directly to the destructor of `TransactionProxy`. Refs: #4286 Closes: prisma/team-orm#391
Remove the boilerplate `Transaction.dispose` method from the public driver adapter interface and move the corresponding functionality directly to the destructor of `TransactionProxy`. Refs: #4286 Closes: prisma/team-orm#391
Remove the boilerplate `Transaction.dispose` method from the public driver adapter interface and move the corresponding functionality directly to the destructor of `TransactionProxy`. Refs: #4286 Closes: prisma/team-orm#391
…4489) Remove the boilerplate `Transaction.dispose` method from the public driver adapter interface and move the corresponding functionality directly to the destructor of `TransactionProxy`. Refs: #4286 Closes: prisma/team-orm#391 Co-authored-by: Miguel Fernández <fernandez@prisma.io>
Fixes transactions not being closed after some errors on the Rust side:
<JsQueryable as TransactionCapable>::start_transactionIt is one of the two reasons that may trigger nested transaction errors with libSQL.
It is also a prerequisite for #4288 (otherwise those errors can leave the libSQL connection in locked state).