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

Possibility of exception handling in the future? #49

Open
lazytype opened this issue May 24, 2020 · 18 comments
Open

Possibility of exception handling in the future? #49

lazytype opened this issue May 24, 2020 · 18 comments
Labels
for follow-on proposal This issue should be investigated for a follow-on proposal.

Comments

@lazytype
Copy link

lazytype commented May 24, 2020

Even though the proposal currently centers the motivation around resource management, the syntax is also an attractive option for performing other patterns that could also clean up JavaScript code. A couple patterns (that will look familiar to Pythonistas) one might want to use in unit tests are:

// Automatic cleanup
using (const spy = jest.withSpy(...)) { // calls `jest.spyOn(...)`
  ...
} // automatically calls `spy.mockRestore()`

// Error suppression and handling
using (expectToThrow(TypeError)) {
  undefined[0];
} // would throw an exception if the block didn't throw a `TypeError`

The latter would require some means of specifying what to do when an exception is thrown. One way this could look is:

function expectToThrow(errorType) {
  return {
    [Symbol.dispose]: (error) => {
      if (error instanceof errorType) {
        // Returning `undefined` suppresses the error
        return;
      }
      // Allows the error to be thrown
      return error;
    }
}

which would behave somewhat like

...
catch ($$error) {
    // Need to distinguish between no thrown error vs `undefined` being thrown
    $$try.hasBlockError = true;
    $$try.blockError = $$error;
}
finally {
    while ($$try.stack.length) {
        const { value: $$expr, dispose: $$dispose } = $$try.stack.pop();
        let $$disposeReturnedError;
        try {
            $$disposeReturnedError = $$dispose.call($$expr, $$try.lastError);
        }
        catch ($$error) {
            $$try.errors.push($$error);
        }

        if (typeof $$disposeReturnedError !== 'undefined') {
            // Always substitute the error if the dispose function returned one
            $$try.hasBlockError = true;
            $$try.blockError = $$disposeReturnedError;
        } else if ($$dispose.length !== 0) {
            // Error is suppressed if the dispose function has an error argument
            $$try.hasBlockError = false;
            $$try.blockError = undefined;
        }
    }
    if ($$try.hasBlockError) {
        $$try.errors.push($$try.blockError);
    }
    if ($$try.errors.length > 1) {
        throw new AggregateError($$try.errors);
    }
    if ($$try.errors.length === 1) {
        throw $$try.errors[0];
    }
}
...

Note that this isn't a request to modify the current proposal to include this behavior. I'm just raising this now to dissuade an API from being shipped that precludes this functionality, should it be desired in the future.

@lazytype
Copy link
Author

lazytype commented May 24, 2020

Also just wanted to throw out the possibility of being able to do something like:

using (retry({maxAttempts: 3, backoff: 1000})) {
  ...
}

where retry adds behavior like:

let attempts = 0;
while (true) {
  try {
    ...
  } catch (e) {
    if (++attempts < maxAttempts) {
      sleep(backoff);
      continue;
    } 
    throw e;
  }
  break;
}

@rbuckton
Copy link
Collaborator

I discussed this in another issue, so I'll post the relevant comment here as well:

Static languages like TypeScript would need to be able to interpret error suppression to provide accurate control-flow analysis

We would probably not add any special logic here. We would either optimistically assume that every statement after a using declaration/inside a using block occurred, or pessimistically assume that it might not have. I don't know about an in-between.
In other words, Suppress wouldn't get any first-class support.

The only way I could see it working is if we had a way to unambiguously know a disposable has suppression capabilities. The easiest way to do that would be with a different global symbol (i.e, Symbol.exitContext), to differentiate between a disposable resource that doesn't have error suppression capabilities vs. one that does:

class BasicDisposable {
  [Symbol.dispose](): void {
    // cleanup, but no suppression
  }
}

class Suppress {
  #filter: (error: unknown) => boolean;
  constructor(filter: (error: unknown) => boolean) {
    this.#filter = filter;
  }

  [Symbol.exitContext](completion: "return" | "throw", cause: unknown) {
    // custom error suppression semantics, all bets are off for CFA
  }
}

{
  using const res1 = new BasicDisposable();
  // res1 has `Symbol.disposable`, CFA can be optimistic
}

{
  using const res2 = new Suppress(() => ...);
  // res2 has `Symbol.exitContext`, CFA should be pessimistic
}

For { [Symbol.dispose](): any } | null | undefined (or { [Symbol.asyncDispose](): any }) we could be optimistic, for anything else we could be pessimistic.

NOTE: Symbol.exitContext is more directly based on Python's __exit__. We could also consider adding Symbol.enterContext in parallel to Python's __enter__ to satisfy #69, if the use case is compelling enough.

This isn't something I'm currently considering for this proposal, but may potentially be considered for a follow-on proposal.

@rbuckton
Copy link
Collaborator

The following was originally posted by @nicolashenry in #74 (comment)

I want to share a use case about error management. I am thinking about using it in the context of a SQL transaction in this way:

using transaction = {
  ...
  [Symbol.dispose](error) {
    if (error) {
      this.rollback();
    } else {
      this.commit();
    }
  }
 };

Or maybe in this way if the use of Symbol.exitContext is retained in the spec.

using transaction = {
  ...
  [Symbol.exitContext](completion, cause) {
    this.completion = completion;
    this.cause = cause;
  }
  [Symbol.dispose]() {
    if (this.completion === 'return') {
      this.commit();
    } else if (this.completion === 'throw') {
      this.rollback();
    }
  }
 };

(It implies Symbol.exitContext method needs to be called before Symbol.dispose method.)

I think the lack of error context could be problematic for this particular use case.

@rbuckton rbuckton added the for follow-on proposal This issue should be investigated for a follow-on proposal. label Oct 5, 2022
@rbuckton
Copy link
Collaborator

rbuckton commented Jul 8, 2023

This was also discussed in #159, so I will summarize my thoughts from that issue here.

There are three different mechanisms we can consider for resource management, though they are not mutually exclusive:

  • Lightweight disposables using Symbol.dispose:
    • In using x = y, x is initialized to the value of y which is the resource itself.
    • Potential to forget using, but also easy to compose multiple resources whose lifetime extends longer than the enclosing block.
  • Disposable producers that use Symbol.create (or some other name, more on that later):
    • In using x = y, x is initialized to the value of y[Symbol.create]() which returns the resource to be disposed.
    • Much harder to forget using as you must manually invoke y[Symbol.create]() to opt-out on the consumer side.
    • Since most existing APIs would likely just implement [Symbol.create]() { return this; }, it is feasible to make this optional for using, and thus opt-in on the producer side.
  • Full context managers that use Symbol.create and Symbol.exitContext to intercept completions.
    • Acts like a disposable producer, but more powerful.
    • Can intercept and swallow exceptions.

Instead of Symbol.create, it's possible that Symbol.enter or Symbol.enterContext might be a better name to cover all cases.

@rbuckton
Copy link
Collaborator

rbuckton commented Jul 8, 2023

In a way you could consider disposable resources have the following implicit behavior:

  • If Symbol.dispose is present, using acts as if an implicit [Symbol.enterContext]() method exists (if not found).
  • If Symbol.dispose is present, using acts as if an implicit [Symbol.exitContext]() method exists (if not found).

The implied [Symbol.enterContext]() method would act something like:

[Symbol.enterContext]() {
  return this;
}

The implied [Symbol.exitContext]() method would act something like:

[Symbol.exitContext](hasError, error) {
  try {
    this[Symbol.dispose]();
  } catch (e) {
    if (hasError) {
      throw new SuppressedError(error, e);
    } else {
      throw e;
    }
  }
  return false; // let the caller throw 'error'
}

@rixtox
Copy link

rixtox commented Jul 19, 2023

I'm not that interested in a full-featured context manager like Python, which allows arbitrarily overwriting or swallowing the thrown error of the exiting scope. In some sense the current proposal already allows overwriting error in dispose method, but in a limited fashion.

However, I do hope to at least allow [Symbol.dispose]() to observe the most recently thrown error of the existing scope. This would enable use cases like db rollback as @nicolashenry mentioned. This change can be added fully compatible with all existing Disposable adoptions.

@rbuckton
Copy link
Collaborator

rbuckton commented Jul 20, 2023

However, I do hope to at least allow [Symbol.dispose]() to observe the most recently thrown error of the existing scope. This would enable use cases like db rollback as @nicolashenry mentioned. This change can be added fully compatible with all existing Disposable adoptions.

I don't think passing the error to @@dispose is a good idea as it is more likely to cause confusion to users as to whether they should rethrow the exception themselves, which would only make the resulting SuppressedError more confusing.

Also, there are more than a few non-exception related reasons to roll back a database transaction, which is why the example in the explainer assumes rollback unless you mark the transaction as succeeded at the end of the block.

@rixtox
Copy link

rixtox commented Jul 20, 2023

Also, there are more than a few non-exception related reasons to roll back a database transaction, which is why the example in the explainer assumes rollback unless you mark the transaction as succeeded at the end of the block.

That approach is very fragile if you bound multiple resources:

// roll back transaction if either action fails
async function transfer(account1, account2) {
  await using tx = transactionManager.startTransaction(account1, account2);
  await using resource = someResource(account1, account2); // resource[Symbol.dispose] throws
  await account1.debit(amount);
  await account2.credit(amount);
  await resource.reduce(amount);

  // mark transaction success if we reach this point
  tx.succeeded = true;
} // await transaction commit or rollback

In this case if the bound resource throws during disposal, tx would have no way to tell that and would not be able to roll back.

@Jamesernator
Copy link

Also, there are more than a few non-exception related reasons to roll back a database transaction, which is why the example in the explainer assumes rollback unless you mark the transaction as succeeded at the end of the block.

This seems very uncompelling to me, usually no errors would mean that everything has succeeded fine. For the case of indexedDB specifically this is specifically from how transactions already work.

Like it just seems like a hazard that one could change const to using here and no longer have a transaction that commits:

{
    const tx = db.transaction("keyval", "readwrite");
    const store = db.getStore("keyval");
    store.put(3, "counter");
}

The spec even specifically says that .commit() need not normally be called.

I don't think passing the error to @@dispose is a good idea as it is more likely to cause confusion to users as to whether they should rethrow the exception themselves

A simpler solution would just be to pass a boolean to dispose, I don't see that the error itself would be particularly useful other than just the signal that disposal is because of an error.

@rbuckton
Copy link
Collaborator

This seems very uncompelling to me, usually no errors would mean that everything has succeeded fine. For the case of indexedDB specifically this is specifically from how transactions already work.

Like it just seems like a hazard that one could change const to using here and no longer have a transaction that commits:

{
    const tx = db.transaction("keyval", "readwrite");
    const store = db.getStore("keyval");
    store.put(3, "counter");
}

The spec even specifically says that .commit() need not normally be called.

Having written more than my fair share of code leveraging RDBMS and distributed transactions over the years, I've run into plenty of occasions where transaction rollback shouldn't and isn't dependent on exceptions. However, I will say that most of these cases have been in languages where dispose-like semantics have no way to observe whether the transaction was closed due to completion or exception, and in those cases there's usually been an explicit way to signal transaction completion with the default being to rollback. For example, .NET's System.Transactions.TransactionScope has a Complete method that is used to indicate all operations completed successfully, while it's Dispose method performs rollback if the transaction was not completed. In addition System.Data.Common.DbTransaction recommends to rollback on dispose (though that is provider specific).

With respect to IndexedDB, it's transactions aren't aware of exceptions to begin with, and its behavior is to auto-commit, which means the safest approach currently would be to use try..catch in any case. I find the "rollback unless success was signaled" approach to be far more reliable with dispose-semantics, but either approach (auto-commit or auto-rollback) can be implemented in terms of the other through the use of a wrapper.

I don't think passing the error to @@dispose is a good idea as it is more likely to cause confusion to users as to whether they should rethrow the exception themselves

A simpler solution would just be to pass a boolean to dispose, I don't see that the error itself would be particularly useful other than just the signal that disposal is because of an error.

I still feel that this introduces more complexity than is justified, and even further weakens the value proposition. I currently favor just recommending try..catch if you need exception details, and feed that into the resource if it needs to perform any kind of recovery action. In lieu of a succeeded property, for an IndexedDB transaction this would just be:

using tx = db.transaction();
try {
  ...
}
catch (e) {
  tx.rollback();
  throw e;
}

It is likely that this what users are already doing today and doesn't introduce confusion as to what to do with an exception when implementing @@dispose, or how to properly call a @@dispose method imperatively.

@rixtox
Copy link

rixtox commented Jul 22, 2023

For example, .NET's System.Transactions.TransactionScope has a Complete method that is used to indicate all operations completed successfully, while it's Dispose method performs rollback if the transaction was not completed. In addition System.Data.Common.DbTransaction recommends to rollback on dispose (though that is provider specific).

In your .NET examples, proper scoping is required.

// Create the TransactionScope to execute the commands, guaranteeing
// that both commands can commit or roll back as a single unit of work.
using (TransactionScope scope = new TransactionScope())
{
    using (SqlConnection connection1 = new SqlConnection(connectString1))
    {
        // Opening the connection automatically enlists it in the 
        // TransactionScope as a lightweight transaction.
        connection1.Open();

        // Create the SqlCommand object and execute the first command.
        SqlCommand command1 = new SqlCommand(commandText1, connection1);
        returnValue = command1.ExecuteNonQuery();
        writer.WriteLine("Rows to be affected by command1: {0}", returnValue);

        // If you get here, this means that command1 succeeded. By nesting
        // the using block for connection2 inside that of connection1, you
        // conserve server and network resources as connection2 is opened
        // only when there is a chance that the transaction can commit.   
        using (SqlConnection connection2 = new SqlConnection(connectString2))
        {
            // The transaction is escalated to a full distributed
            // transaction when connection2 is opened.
            connection2.Open();

            // Execute the second command in the second database.
            returnValue = 0;
            SqlCommand command2 = new SqlCommand(commandText2, connection2);
            returnValue = command2.ExecuteNonQuery();
            writer.WriteLine("Rows to be affected by command2: {0}", returnValue);
        }
    }

    // The Complete method commits the transaction. If an exception has been thrown,
    // Complete is not  called and the transaction is rolled back.
    scope.Complete();
}

We need to do the same in JavaScript:

async function transfer(account1, account2) {
  await using tx = transactionManager.startTransaction(account1, account2);
  {
    await using resource = someResource(account1, account2); // dispose may throw
    await account1.debit(amount);
    await account2.credit(amount);
    await resource.reduce(amount);
  }
  // mark transaction success if we reach this point
  tx.succeeded = true;
}

I think using is complicating the control flow for code block when used in this way. The code block may appear redundant to developers, but can actually affect the grouping of error capturing, and in this case it's guarding the inner resource disposal from throwing after tx.successed = true. This will create a lot more confusion for developers.

@rixtox
Copy link

rixtox commented Jul 22, 2023

Another way is to learn from how Golang handles error recovery using recover(). So we can have a static function like:

interface ErrorConstructor {
    recover(): ErrorRecord;
}
interface ErrorRecord {
    hasError: boolean;
    error: any;
}

Calling it would swallow any error thrown in the currently disposing stack.

@rbuckton
Copy link
Collaborator

The .NET example predates C#'s own addition of using and await using declarations. The exact same sample can be written as the following in C# 8.0 or later:

// Create the TransactionScope to execute the commands, guaranteeing
// that both commands can commit or roll back as a single unit of work.
using TransactionScope scope = new TransactionScope();
using SqlConnection connection1 = new SqlConnection(connectString1);

// Opening the connection automatically enlists it in the
// TransactionScope as a lightweight transaction.
connection1.Open();

// Create the SqlCommand object and execute the first command.
SqlCommand command1 = new SqlCommand(commandText1, connection1);
returnValue = command1.ExecuteNonQuery();
writer.WriteLine("Rows to be affected by command1: {0}", returnValue);

// If you get here, this means that command1 succeeded. By nesting
// the using block for connection2 inside that of connection1, you
// conserve server and network resources as connection2 is opened
// only when there is a chance that the transaction can commit.
using SqlConnection connection2 = new SqlConnection(connectString2);

// The transaction is escalated to a full distributed
// transaction when connection2 is opened.
connection2.Open();

// Execute the second command in the second database.
returnValue = 0;
SqlCommand command2 = new SqlCommand(commandText2, connection2);
returnValue = command2.ExecuteNonQuery();
writer.WriteLine("Rows to be affected by command2: {0}", returnValue);

// The Complete method commits the transaction. If an exception has been thrown,
// Complete is not  called and the transaction is rolled back.
scope.Complete();

@rbuckton
Copy link
Collaborator

Or as follows, if you want to preserve the block scoping:

// Create the TransactionScope to execute the commands, guaranteeing
// that both commands can commit or roll back as a single unit of work.
using TransactionScope scope = new TransactionScope();
{
    using SqlConnection connection1 = new SqlConnection(connectString1);

    // Opening the connection automatically enlists it in the
    // TransactionScope as a lightweight transaction.
    connection1.Open();

    // Create the SqlCommand object and execute the first command.
    SqlCommand command1 = new SqlCommand(commandText1, connection1);
    returnValue = command1.ExecuteNonQuery();
    writer.WriteLine("Rows to be affected by command1: {0}", returnValue);

    {
        // If you get here, this means that command1 succeeded. By nesting
        // the using block for connection2 inside that of connection1, you
        // conserve server and network resources as connection2 is opened
        // only when there is a chance that the transaction can commit.
        using SqlConnection connection2 = new SqlConnection(connectString2);

        // The transaction is escalated to a full distributed
        // transaction when connection2 is opened.
        connection2.Open();

        // Execute the second command in the second database.
        returnValue = 0;
        SqlCommand command2 = new SqlCommand(commandText2, connection2);
        returnValue = command2.ExecuteNonQuery();
        writer.WriteLine("Rows to be affected by command2: {0}", returnValue);
    }
}

// The Complete method commits the transaction. If an exception has been thrown,
// Complete is not  called and the transaction is rolled back.
scope.Complete();

@rbuckton
Copy link
Collaborator

@rixtox I would suggest you read through https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-8.0/using#using-declaration if you would like to learn more about C#'s adoption of using declarations.

@rixtox
Copy link

rixtox commented Jul 22, 2023

@rbuckton
The .NET example predates C#'s own addition of using and await using declarations. The exact same sample can be written as the following in C# 8.0 or later:

// Create the TransactionScope to execute the commands, guaranteeing
// that both commands can commit or roll back as a single unit of work.
using TransactionScope scope = new TransactionScope();
using SqlConnection connection1 = new SqlConnection(connectString1);

What I was getting to is, if I want to roll back if the second using throws in dispose, then you have to wrap it inside a separate block scope. This is also related to #197 as an implication of adopting RAII.

In this case what you are claiming is incorrect. Your re-written sample is not "exactly the same". It behaves differently than the original sample when the second using throws in dispose. Specifically:

public class TransactionScope : IDisposable
{
    public void Complete()
    {
        Console.WriteLine("Complete");
    }
    public void Dispose()
    {
    }
}

public class SqlConnection : IDisposable
{
    public void Dispose()
    {
        throw new Exception();
    }
}

class OriginalSample
{
    static void Main()
    {
        using (TransactionScope scope = new TransactionScope())
        {
            using (SqlConnection connection1 = new SqlConnection())
            {
            }
            scope.Complete();
        }
    }
}

class RewrittenSample
{
    static void Main()
    {
        using TransactionScope scope = new TransactionScope();
        using SqlConnection connection1 = new SqlConnection();
        scope.Complete();
    }
}

The original sample won't call Complete() while your written sample will call Complete() in this case.

The C# compiler knows they have different control flow, and outputs very different compiled code after inlining and dead code elimination: https://godbolt.org/z/1c35365jY

If even you can get this wrong, I don't expect any developer can get it right.

I would suggest you read through https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/proposals/csharp-8.0/using#using-declaration if you would like to learn more about C#'s adoption of using declarations.

Even this document you linked is overlooking this problem and claims that these are equivalent, which they are not.

if (...) 
{ 
   using FileStream f = new FileStream(@"C:\users\jaredpar\using.md");
   // statements
}

// Equivalent to 
if (...) 
{ 
   using (FileStream f = new FileStream(@"C:\users\jaredpar\using.md")) 
   {
    // statements
   }
}

It's only true if statements are all statements for the rest of the block, or if the statements contain no using statements.

@rixtox
Copy link

rixtox commented Jul 24, 2023

The very fact that the original sample put the scope.Complete() statement outside the second using scope indicated the intention of skipping it in case the second using failed in dispose. Otherwise the original sample could just write:

using (TransactionScope scope = new TransactionScope())
{
    using (SqlConnection connection1 = new SqlConnection())
    {
        scope.Complete();
    }
}

If it was in this form, then your rewritten sample would have bahaved the same. But you overlooked the intention behind that original sample, and changed its behavior without noticing it. This is exactly what I want to point out as a danger in adopting RAII.

The situation in C# is even worse, because it has two different syntax for using with different behaviors but dangerously similar lexical appearance. This is a nightmare for code review. Just take a look at these different programs and try to identify those with the same behaviors:

{
    using (TransactionScope scope = new TransactionScope())
    {
        using (SqlConnection connection1 = new SqlConnection())
        {
            connection1.Open();
        }
        scope.Complete();
    }
}

{
    using TransactionScope scope = new TransactionScope();
    {
        using SqlConnection connection1 = new SqlConnection();
        {
            connection1.Open();
        }
    }
    scope.Complete();
}

{
    using (TransactionScope scope = new TransactionScope())
    {
        using (SqlConnection connection1 = new SqlConnection())
        {
            connection1.Open();
            scope.Complete();
        }
    }
}

{
    using (TransactionScope scope = new TransactionScope())
    {
        using SqlConnection connection1 = new SqlConnection();
        {
            connection1.Open();
        }
        scope.Complete();
    }
}

{
    using (TransactionScope scope = new TransactionScope())
    {
        using SqlConnection connection1 = new SqlConnection();
        {
            connection1.Open();
            scope.Complete();
        }
    }
}

{
    using (TransactionScope scope = new TransactionScope())
    {
        using SqlConnection connection1 = new SqlConnection();
        connection1.Open();
        scope.Complete();
    }
}

{
    using TransactionScope scope = new TransactionScope();
    {
        using SqlConnection connection1 = new SqlConnection();
        connection1.Open();
        scope.Complete();
    }
}

{
    using TransactionScope scope = new TransactionScope();
    using SqlConnection connection1 = new SqlConnection();
    connection1.Open();
    scope.Complete();
}

@rixtox
Copy link

rixtox commented Jul 26, 2023

This is actually a common problem for languages adopting the RAII mechanism. In C++11, destructors are implicitly annotated with noexcept(true), meaning an escaped exception from the destructor will terminate the program unless noexcept(false) is used to override the behavior. However, even with noexcept(false), if the unwinding stack already has an unhandled exception, an additional exception escaping a destructor would still terminate the program.

In another word, C++ language designers explicitly made exceptions not suppressible. (Not to confuse suppressing error with std::nested_exception though)

As a result, many people recommend against throwing from destructors. For example there are many similar arguments in this discussion. std::fstream for example also makes sure no error is thrown if the destructor is closing the file. Some code styling guides like Google's even outright banned all exceptions in favor of explicit error handling.

There are some defer-like alternatives for C++ like stack_unwinding, folly's ScopeGuard, and Boost.ScopeExit. These still bind to lexical scope, but are way more explicit than RAII.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
for follow-on proposal This issue should be investigated for a follow-on proposal.
Projects
None yet
Development

No branches or pull requests

4 participants