Suppressed exceptions during dispose

[sosoba]

Imagine that as you catch the main exception, you get more exceptions during implicit calls to dispose methods (ex. remote filestystem has been lost):

async function copy(first, second) {
  using(let firstFile = await open(first)) {
    using(let otherFile = await open(second)) {
      if( someCondition ) {
        throw new Error( 'Some error' );
      }      
    }
  }
}

try {
  await copy( 'first.txt', 'second.txt' );
} catch (e) {
  console.log( e.message ); //  'Some error'
  console.log( e.suppressed ); // [ Error('Device unavailable'), Error('Broken pipe') ]
}

In Java, implicit exceptions are suppressed and added to the main. Is a similar solution planned here?

[rbuckton]

Per the explainer, exceptions thrown during dispose while already handling an exception are aggregated and thrown as an AggregateError, so given your example:

try {
  await copy( 'first.txt', 'second.txt' );
} catch (e) {
  console.log( e instanceof AggregateError ); // true
  console.log( e.errors ); // [ Error('Some error'), Error('Device unavailable'), Error('Broken pipe') ]
}

You can see an example of this in the transposed representation here: https://github.com/tc39/proposal-explicit-resource-management#try-using-with-explicit-local-bindings

[sosoba]

I suppose aggregation is optional?

try {
  await copy( 'first.txt', 'second.txt' );
} catch (e) {
  let main, suppressed;
  if ( e instanceof AggregateError ) {
    main = e.errors[0];
    suppressed = e.errors.slice(1);
  } else {
    main = e;
    suppressed = [];
  }
  console.log( main ); //  'Some error'
  console.log( suppressed ); // [ Error('Device unavailable'), Error('Broken pipe') ]
}

[rbuckton]

Yes, aggregation happens purely based on whether more than one error is thrown (whether by the user or during cleanup), so it would be necessary to check the error. The issue with using a .suppressed property is that JS errors are not explicitly typed to Error, which means that its possible to throw "foo", 1, null, etc. as an exception and you can't attach a .suppressed to that. Its also possible to throw an error that is frozen, which also cannot be modified. Throwing AggregateError, while less than ideal in some situations, is more reliable here.

[ljharb]

That seems confusing; why not always make it an aggregate error?

Promise.allSettled doesn't skip the AggregateError when there's only one rejection; that forces consumers to duck-type and have two code paths.

[rbuckton]

That seems confusing; why not always make it an aggregate error?

Promise.allSettled doesn't skip the AggregateError when there's only one rejection; that forces consumers to duck-type and have two code paths.

Promise.allSettled is slightly different, in that it is an API call that generally expects more than one input. However, would you expect the following to produce an AggregateError in all cases? Consider the following three scenarios:

Scenario A - Exception in other code but no exception from dispose

try {
  using const x = new Disposable(() => {}); // does nothing, won't throw
  throw new Error();
}
catch (e) {
  console.log(e.name); // should this be Error or AggregateError?
}

Scenario B - Exception in dispose but no exception from other code

try {
  using const x = new Disposable(() => { throw new Error(); }
  // no other code that could error
}
catch (e) {
  console.log(e.name); // should this be Error or AggregateError?
}

Scenario C - Multiple exceptions in dispose but no exception from other code*

try {
  using const x = new Disposable(() => { throw new Error(); }
  using const y = new Disposable(() => { throw new Error(); }
  // no other code that could error
}
catch (e) {
  console.log(e.name); // this should definitely be AggregateError
}

Scenario D - Exception in dispose and in other code

try {
  using const x = new Disposable(() => { throw new Error("in dispose"); }
  throw new Error("in other code");
}
catch (e) {
  console.log(e.name); // What should this be?
}

There are multiple ways of addressing these scenarios:

1. Only throw AggregateError if there's more than one error:
   • A and B print Error
   • C and D print AggregateError
2. Always throw AggregateError:
   • A, B, C, and D print AggregateError
3. Only throw AggregateError if there are one or more exceptions during dispose (include original error if present)
   • A prints Error
   • B, C, and D print AggregateError
4. Throw AggregateError if there are only exceptions during dispose. If original error thrown from user code, rethrow with a suppressed property (per OP):
   • A prints Error
   • B and C print AggregateError
   • D prints Error

Currently, the proposal uses (1). (2) does not make sense to me if all of the disposals complete successfully. I might be convinced to consider (3), but there are issues with (4) I mentioned in my previous comment (i.e., in JS you can throw anything, including a frozen Error instance).

The only way I could see something like (4) working is via a meta-property that's only accessible in a catch clause (i.e., throw.suppressed or catch.suppressed), or having logic that wraps a non-Error/non-mutable exception in a fresh Error.

[rbuckton]

... though if we did go the meta-property route, it would be nice to have other meta-properties that could be used in finally as well:

• throw.hasError — true in a catch clause, or in a finally block if propagating an uncaught error; false any other time.
  • Alternatives: .errored, .hasFault, .faulted, .hasException, .excepted, .wasThrown, etc.
• throw.error — The thrown error that caused us to jump to catch or finally (if there was one).
  • Alternatives: .fault, .exception, .thrown, .cause, .reason, etc.
• throw.suppressed — An AggregateError for any errors thrown during disposal.

I've had more than a few cases where I've written something like this:

let ok = false;
try {
  ...
  ok = true;
}
finally {
  if (!ok) { /* exception was thrown */ }
}

It would also be nice to have just a plain throw; statement in a catch clause that rethrows the caught exception (C# has this, not sure about other languages offhand). Might be worth a separate proposal, and I've been thinking about it since we added bindingless catch:

try {
  ...
}
catch {
  // do something that cares about the fact we failed but doesn't need the error itself
  throw; // rethrow unmodified exception
}

[ljharb]

I would expect A to be Error, since that's what's thrown; B and C i would expect to be the same kind of error, since whatever it is should connote that it's from disposal (not ONLY from disposal, of course; but all disposals i'd expect to throw the same kind of error); D should match B and C, since the disposal error (like an error in a finally) trumps the error from the try body.

A metaproperty wouldn't work because thrown exceptions can also reject promises, and a rejected promise should have access to the same newly-available information that a catch block does.

[rbuckton]

So, your suggestion would be to wrap everything in an AggregateError if anything throws during disposal, but just propagate the original error if there are no errors during disposal?

A metaproperty wouldn't work because thrown exceptions can also reject promises, and a rejected promise should have access to the same newly-available information that a catch block does.

Fair enough, though I still might consider proposing the other metaproperties and throw; someday when my plate isn't so full.

[ljharb]

Yes, that would be my suggestion. I suppose a DisposalError with a cause being the AggregateError would be clearer, but i doubt adding another error type would be palatable.

[rbuckton]

I can make that change. I do have a related question though. In Disposable.from(iterable), I've set up the semantics such that:

• If an error is thrown during IteratorStep or IteratorValue, all disposables collected so far are disposed and the outer error is thrown (possibly as an AggregateError if any of the disposables also threw during dispose).
  • Essentially, if there is an error calling .next() or reading value or done, then iteration should stop.
• Errors thrown while reading [Symbol.dispose] from each iterated value are collected until iteration ends. At that point, all valid collected resources are disposed and the collective set of errors are thrown as an AggregateError.
  • This means that iteration is able to continue successfully, so we should continue to consume disposables so that all are disposed when we eventually report the error for a missing [Symbol.dispose].

Would that match your intuition?

[ljharb]

I think so?

In the case when a body error and disposal error are both thrown, i'd hope that all of those errors are available somehow on the aggregate error - perhaps the body error could be the cause and the disposal errors could be the errors.

[rbuckton]

Having both cause and errors seems strange, as an AggregateError generally represents all the errors and anyone who runs into an AggregateError today generally is just looking at the .errors property. If the body error was the cause, would it also exist in errors? I feel like this could be a source of confusion either way.

try {
  using const x = new Dispose(() => { throw Error("in dispose"); });
  throw Error("in body");
}
catch (e) {
  e.cause; // `Error("in body")` (?)
  e.errors; // `[Error("in dispose")]` or `[Error("in body"), Error("in dispose")]`?
}

try {
  using const x = new Dispose(() => { throw Error("in dispose"); });
}
catch (e) {
  e.cause; // `undefined` (?)
  e.errors; // `[Error("in dispose")]`
}

Error cause is fairly new compared to AggregateError, so I'm not sure if everyone's intuition would match. If I had to use cause, I think my preference would be for the body error to be in both cause and errors (since it was the original error and is one of multiple errors thrown from the block), but I'm not completely sure.

[ljharb]

In my suggestion, the body error was in .cause and NOT in .errors; if it's in .errors then there's no point setting a cause.

In other words, I think it would either be both of those examples, or, the body error would be the first item in .errors. In the latter case tho, how would you know when catching that error if the errors you're looking at contains a body error or not?

In other words, it seems cleaner to me to somehow differentiate the 0 or 1 body error, from the 0 or N disposal errors, in a more explicit way than "maybe it's the 0th item?"

[rbuckton]

My source of concern is current expectations around how AggregateError generally works. If this were a separate error I would probably be less concerned. I think I am fine with the body error being cause however, since this is new syntax and MDN or other sites could document what to expect.

[ljharb]

Right - in the case where both the body and disposal throws (the other cases are easy), the choices are:

1. deliver both errors in separate channels
2. deliver both errors smooshed together in errors
3. drop one of the errors on the floor

The third seems very bad, the second seems suboptimal, the first is unfortunately awkward, and .cause was just the first thing I thought of to achieve it.

[rbuckton]

I went with AggregateError where errors are the errors from dispose and cause is the body error (if present). If there are no errors from dispose, the body error is thrown directly.

Fixed in 7968c49