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

Proposal: DOMException.ignoreAborts(promise) #933

Open
domenic opened this issue Oct 29, 2020 · 17 comments
Open

Proposal: DOMException.ignoreAborts(promise) #933

domenic opened this issue Oct 29, 2020 · 17 comments

Comments

@domenic
Copy link
Member

domenic commented Oct 29, 2020

Ported from whatwg/dom#881.

Consider the following code we're adding as an example to the Streams Standard in whatwg/streams#1059:

const controller = new AbortController();
const pipePromise = readable1.pipeTo(writable, { preventAbort: true, signal: controller.signal });

// ... some time later ...
controller.abort();

// Wait for the pipe to complete before starting a new one:
try {
 await pipePromise;
} catch (e) {
 // Swallow "AbortError" DOMExceptions as expected, but rethrow any unexpected failures.
 if (e.name !== "AbortError") {
  throw e;
 }
}

// Start the new pipe!
readable2.pipeTo(writable);

Wouldn't it be nice if we could do this instead?

const controller = new AbortController();
const pipePromise = readable1.pipeTo(writable, { preventAbort: true, signal: controller.signal });

// ... some time later ...
controller.abort();
await DOMException.ignoreAborts(pipePromise);

// Start the new pipe!
readable2.pipeTo(writable);

The semantics here are roughly

DOMException.ignoreAborts = function (promise) {
  promise = Promise.resolve(promise);

  return promise.catch(e => {
    if (isDOMException(e) && e.name === "AbortError") {
      // swallow exception
    } else {
      // rethrow
      throw e;
    }
  });
};

This is small enough that I'd be willing to implement it in Chromium, if given the binding team's blessing. (@yuki3, thoughts?)

Any thoughts from other implementers?

@yuki3
Copy link

yuki3 commented Oct 29, 2020

From a point of view of Blink-V8 bindings, IIUC, you're going to implement a new (static?) IDL operation (which takes a promise and returns a promise) and no new mechanism seems necessary. Then, no objection of course. (The prototype chain of DOMException is a bit special, but I don't think it matters in this case.)

I'm sorry that I'm not familiar with this sort of code and honestly I don't understand its usefulness. Probably it's very common, I guess? Only my question is that AbortError is so special? Do we want to ignore other kinds of errors? Like ignoreErrors(promise, [error_name1, error_name2, ...])? I have no strong feeling.

@domenic
Copy link
Member Author

domenic commented Oct 29, 2020

From a point of view of Blink-V8 bindings, IIUC, you're going to implement a new (static?) IDL operation (which takes a promise and returns a promise) and no new mechanism seems necessary.

Yep! I just wanted to check, since you all are really the best "owners" for DOMException.

I'm sorry that I'm not familiar with this sort of code and honestly I don't understand its usefulness. Probably it's very common, I guess? Only my question is that AbortError is so special

Yeah, "AbortError" is quite special, because it's a type of error that's often expected by web developers who have aborted something, and they want to ignore it. (Things they could abort would be, e.g., a fetch, or a stream pipe.)

Some of the original designs for promise cancelation (what is now AbortSignal) actually used a new type of completion value, not an exception and not a return value, to represent cancelations. Instead we ended up reusing the exception channel, but this leads to awkward code like the above where you want to treat most exceptions as exceptions, but ignore "AbortError"s since they represent a cancelation.

@annevk
Copy link
Member

annevk commented Oct 30, 2020

Shouldn't throw new TypeError(); be return Promise.reject(new TypeError()) or some such? I wonder if this static should be on AbortSignal or AbortController instead as it directly relates to them.

@domenic
Copy link
Member Author

domenic commented Oct 30, 2020

@annevk you're right, it should; will update the OP.Actually, per Web IDL rules, it should Promise.resolve() its argument. I've updated the OP in that direction.

I think a reasonable argument could be made for any of DOMException, Promise, AbortSignal, or AbortController. Just in terms of what code the author writes, DOMException.ignoreAborts() seems pretty clean to me. Also it's a bit nicer in reducing coupling; right now AbortSignal doesn't "know" about DOMException.

@bathos
Copy link
Contributor

bathos commented Oct 30, 2020

I think this is a great idea, though I realized after looking at some places in our codebase where we’re doing “isAbortError” checks that it seems like it might be a more niche thing than I initially supposed (or maybe we’re just weird) and that it may not be helpful in our case.

It seemed like the most common places where we have these checks is in async functions where control flow should move to the catch block if an abort occurs. If I’m not mistaken, if we tried to leverage this API, we’d end up having to move the error handling that occurs within these catch blocks elsewhere in each such case — and it might end up being a net loss for removing boilerplate / improving clarity.

async butterBiscuits({ signal }) {
  try {
    this.biscuits.butter = await fetch('https://butternet/the-good-stuff?lots', { signal });
  } catch (err) {
    if (!isAbortError(err)) {
      alerts.push('a TERRIBLE thing happened');
      throw err;
    }
  }
}

Note that if the fetch here aborts, we wouldn’t want this.biscuits.butter to be set to undefined. Sometimes there are sequentially or simultaneously awaited-things; sometimes there’s more complex handling based on e.g. HTTP status codes, etc. But in all cases I found, we’d never want DOMException.ignoreAborts(aSpecificPromiseWithinTheOverallLogic) inside the try block portion — it’s only the “top level” async logic that wants to ignore aborts.

Is there a pattern I’m missing where ignoreAborts would provide a benefit for cases like that? (Of course, there doesn’t have to be; it could be that this API is tailored specifically for cases like the initial example where you want to continue after a “child” promise gets aborted, which is still useful.)

@domenic
Copy link
Member Author

domenic commented Oct 30, 2020

@bathos, your example would be rewritten as follows:

async butterBiscuits({ signal }) {
  try {
    const result = await DOMException.ignoreAborts(fetch('https://butternet/the-good-stuff?lots', { signal }));
    if (result) {
      this.biscuits.butter = result;
    }
  } catch (err) {
    alerts.push('a TERRIBLE thing happened');
    throw err;
  }
}

@bathos
Copy link
Contributor

bathos commented Oct 30, 2020

Right — so the if-statement “artifact” gets moved up, but hasn’t actually gone away. And although that example doesn’t illustrate it cause it was 1:1, it multiplies it, too — an additional new if statement for each abortable-awaited-thing, instead of just one for all of them.

@Jamesernator
Copy link

I personally think it would be most natural as a method of Promise. This would have an added benefit of integrating really nicely with the await.ops proposal e.g.:

await Promise.ignoreAbort(somePromise);

// with the proposal
await.ignoreAbort somePromise;

And as an added benefit it would be a less-awkward thing to perhaps standardize later in the TC39 than "DOMException" which is really weird legacy name that confers little meaning (especially in this context).

@domenic
Copy link
Member Author

domenic commented Nov 21, 2020

This doesn't make much sense on Promise since it's specifically about ignoring "AbortError" DOMExceptions.

@TimothyGu
Copy link
Member

Here're my thoughts. Can this be a userland function instead? I can imagine people asking for other related functions, like say DOMException.ignoreNotFound meaning "only bubble up errors if they are not not-found errors". In any case, the function is probably much easier to write in JavaScript than C++.

@annevk
Copy link
Member

annevk commented Nov 23, 2021

@domenic @MattiasBuelens how has the thinking evolved here now that AbortSignal is essentially an "ExceptionSignal"?

@MattiasBuelens
Copy link
Contributor

MattiasBuelens commented Nov 23, 2021

You're right, it's no longer sufficient to only ignore AbortError DOMExceptions. Following the suggestion in whatwg/dom#1027 (review), we would need something along the lines of:

AbortSignal.prototype.ignoreAborts = function (promise) {
  promise = Promise.resolve(promise);

  return promise.catch(e => {
    if (this.aborted && e === this.reason) {
      // swallow exception
    } else {
      // rethrow
      throw e;
    }
  });
};

which authors could use as:

async function butterBiscuits({ signal }) {
  try {
    const result = await signal.ignoreAborts(fetch('https://butternet/the-good-stuff?lots', { signal }));
    if (result) {
      this.biscuits.butter = result;
    }
  } catch (err) {
    alerts.push('a TERRIBLE thing happened');
    throw err;
  }
}

It's kind of weird that you have to use signal twice, though. 😕

@Jamesernator
Copy link

It's kind of weird that you have to use signal twice, though. 😕

I think in most cases you would only bother with it at the same level you create the AbortSignal, i.e. if you're just using a delegated signal you didn't create you'd usually just want to delegate error handling to the caller (as they raised the error).

@annevk
Copy link
Member

annevk commented Nov 23, 2021

@MattiasBuelens thanks, though it seems such an approach would still place special value on aborting. Also, e === this.reason would only work if there was no cloning involved.

Perhaps @yuki3's #933 (comment) is more applicable now, especially if we also introduce AbortSignal.timeout() (which would result in a "TimeoutError" DOMException). In essence it seems you want the ability to filter out certain exceptions. The tricky aspect to something like that would be bridging JS exceptions and DOMException somehow. Or perhaps each can have its own API and they can work together through chaining.

@domenic
Copy link
Member Author

domenic commented Nov 23, 2021

I still think DOMException.ignoreAborts(), which only ignores "AbortError" DOMExceptions, is a good idea. As I've stated over in whatwg/dom#1033 (comment), I don't think the fact that you can change the reason, changes how consumers should treat the result. Most cases don't want a general "ignore any abort reason"; they want "ignore "AbortError" DOMException".

In particular, given the motivating case for branching out, i.e. "TimeoutError" DOMException, you generally don't want to ignore timeouts for a fetch. You want to tell the user: "the operation timed out". Whereas you do want to ignore aborts: the user is probably the one that caused an abort.

@bathos
Copy link
Contributor

bathos commented Nov 23, 2021

you generally don't want to ignore timeouts for a fetch

That's consistent with our experience. Timeout is a result.

We do actually have checks like this in other cases, including one spot where there are three DEX types that are expected and not considered exceptional:

      let shouldIgnore =
        check.isAbortError(err) ||
        check.isNotAllowedError(err) ||
        check.isNotSupportedError(err);

But this example only makes sense in its very local context. The way AbortError models a type of completion instead of an abrupt result seems special / a general semantic related to lower level concerns than the others.

It's like break forced to wear a humiliating throw costume that makes every catch bouncer stop it at the door for an ID check. Or something? This justifies privileged API vis a vis other DOMException types.

@benjamingr
Copy link
Member

Hmm, Note that this was quite the fuss in the dotnet world a bit back dotnet/runtime#21965

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

No branches or pull requests

8 participants