Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Possible to tell whether an error occurred in the body or not? #234

Closed
Vanilagy opened this issue Jun 22, 2024 · 8 comments
Closed

Possible to tell whether an error occurred in the body or not? #234

Vanilagy opened this issue Jun 22, 2024 · 8 comments

Comments

@Vanilagy
Copy link

Vanilagy commented Jun 22, 2024

Not sure how to better name this issue and if the answer is already contained in this repo - I wasn't able to find anything.


I am interested to know if I can use using to model a database transaction API, like this:

async function addPost() {
  await using transaction = await beginTransaction(); // `await using` as finalizing the transaction itself is asynchronous too
  await transaction.createNewPost('hello world');
}

I'd like to obtain this behavior:

  • If the code completes, the dispose call of transaction should commit the changes to the database.
  • If the code fails (error in JS, foreign key violation, etc), the dispose call of transaction should roll back the changes.

Would be awesome if this was possible as it removes a level of indentation that I currently use to model this. However, for this to work, the dispose function would somehow need to know if it's being called after a successful run or due to an early exit caused by an error. Is this possible?

If it's not possible, the next best thing I could think of is:

async function addPost() {
  await using transaction = await beginTransaction(); // `await using` as finalizing the transaction itself is asynchronous too
  await transaction.createNewPost('hello world');

  await transaction.commit();
}

Then, when disposing, the transaction can check if it has been committed. If not, the commit statement has not been reached, implying an error occurred (or implying the programmer just forgot to call commit, which is an error I'd like to make impossible).

But this approach is suboptimal, especially when the function has more than one exit point, such as early returns. The programmer would need to remember to commit at each exit point.

@rbuckton
Copy link
Collaborator

rbuckton commented Jun 23, 2024

The way I would normally model this is by having a method or property that indicates the transaction completed successfully:

async function addPost() {
  await using transaction = await beginTransaction();
  await transaction.createNewPost('hello world');
  transaction.setComplete();
  // or, 
  // transaction.succeeded = true;
}

Marking a transaction as successful indicates whether to commit or rollback when the transaction is disposed. If you never reach the success line, the transaction performs auto-rollback.

This is similar to how DisposableStack works where, if you complete an operation and want the contents of the stack to survive the using, you would call stack.move() to move the contents out of the stack and into a fresh DisposableStack.

Note that this does differ from IDBTransaction, which auto-commits.

@Vanilagy
Copy link
Author

I see, I'll explore the approach you suggested!

So, I take it my original usage is not possible with this feature, where the dispose call knows (perhaps through context that's passed in) what has led up to it being called. That's fine, just wondering, was it intentionally not designed this way?

@pauldraper
Copy link

IMO dispose ought to receive the error.

@pauldraper
Copy link

pauldraper commented Jul 10, 2024

For example, this proposal is currently unsuitable for managing a database transaction.

The usual pattern (this example comes from pg-promise):

await db.tx(async transaction => {
    // transaction body
});

If an error is thrown during the transaction - a database error or otherwise -- the transaction is rolled back. Otherwise it is committed.


Transactions are a canonical use case of resource management. It's disappointing this common pattern cannot actually leverage this proposal as is. I recommend revisiting that.

The obvious solution is to pass the error as an argument.

{
    [Symbol.asyncDispose]: (error) => {}
}

@Vanilagy
Copy link
Author

Agreed! And it just doesn't get passed if there is none. I assume this proposal is too hardened to change this now?

@rbuckton
Copy link
Collaborator

For example, this proposal is currently unsuitable for managing a database transaction.

The usual pattern (this example comes from pg-promise):

await db.tx(async transaction => {
    // transaction body
});

If an error is thrown during the transaction - a database error or otherwise -- the transaction is rolled back. Otherwise it is committed.

Transactions are a canonical use case of resource management. It's disappointing this common pattern cannot actually leverage this proposal as is. I recommend revisiting that.

The design you referenced is designed in this way since it can leverage a callback. It is not necessarily an implicit behavior of a transaction itself. In languages like C#, you use tx.SetComplete() to indicate success.

Languages like Python allow you to observe an exception, but also allow you to act on the exception, including possibly suppressing it. While we do have #49 for discussion of this capability, there are some on the committee (myself included) that are uncomfortable with the complexity that might add.

As a result, there is a line that must be drawn between what we do and don't want [Symbol.dispose] to do. It does not receive an exception because you are limited in what you can do with it, aside from just knowing that an exception was raised. In most use cases that information isn't relevant. For transactions, a different approach (using something like setComplete()) is flexible enough.

I'd much rather defer passing an exception to something like #49, which we can revisit in a future proposal if necessary.

@rbuckton
Copy link
Collaborator

I'm closing this issue. For further discussion on this topic I would suggest you refer to #49.

@Vanilagy
Copy link
Author

Thanks for the pointers, @rbuckton!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants