Specify that AbortController#abort() be bound to AbortController instance

[broofa]

The AbortController/Signal pattern is a bit cumbersome to use because it requires referencing the abort property through the controller instance. If abort() was bound to the controller it could be destructured immediately upon creation. For example, take this use case where the abort() is delegated to an external control...

const abortController = new AbortController();

// delegate abort
$('#someButton').addEventListener('click', () => abortController.abort());

// abortable actions
await someAction();
if (abortController.signal.aborted) throw Error('Aborted');
await otherAction(abortController.signal);

// etc...

Making the suggested change would allow for this code, instead...

const {signal, abort} = new AbortController();

// delegate abort
$('#someButton').addEventListener('click', abort);

// abortable actions
await someAction();
if (signal.aborted) throw Error('Aborted');
await otherAction(signal);

[broofa]

Editorial: The distinction between AbortController and AbortSignal feels overly contrived to me. I get (assume) this pattern is meant to separate the concerns of "signal that can be aborted" from "thing doing the aborting", which I agree may be important at times. However in most use cases this isn't likely to be a significant concern. Thus, having to deal with the controller-signal distinction is more an annoyance than a benefit. Allowing for this form of destructuring would help alleviate this little pain point for people.

[annevk]

This is how all instance methods in the platform behave and changing this would require changing IDL, as far as I know. I somewhat doubt there is appetite for that. There was some kind of JavaScript proposal at one point for automatic/easier bind.

[broofa]

How does one reconcile the fact that same argument could be made for the Console Standard, yet...

const log = console.log;
log('hello world');

... works everywhere?

[Edit to add: Is this the difference between a namespace and an interface in IDL?]

[domenic]

There are no instances of console; it's just a namespace, with only one log method per global. There are many instances of AbortController that need to all use prototypes to share the same method.

[broofa]

Thanks for the quick replies and the explanations here. I do appreciate the time and effort you folks put into these standards.

That said, having made an earnest attempt to use this API I ended up going with the following utility method. It's just easier / simpler to use... unfortunately.

function createAbortable() {
  const signal = { aborted: false };

  return {
    signal,
    abort: () => {
      signal.aborted = true;
    }
  };
}

Read into this what you will, I guess. 🤷

[annevk]

Except that it won't interoperate with web platform APIs? I'm not sure why you closed this, but I could see offering a static that offers the functionality you're after, though ideally we'd do some research first to see if it's a common pattern.

[broofa]

@annevk Thanks for the comments.

I'm realizing that a) closing this may have been a bit premature and b) the problem isn't that abort() needs to be bound to the controller instance, it's that it needs to be bound to the signal instance.

Your comment has helped me realize that what is really bothering me here is that the AbortController is simply not necessary. It's whole raison d'etre is to bind abort() to signal. That is literally all it does. signal instance, abort() method, nothing else.

As such, it is wholly redundant with the many JS language features that allow for such bindings (closures, bind(), =>, etc). Moreover it doesn't actually bind the two together, as evidenced by the fact AbortControllers can be destructured in ways that render the resulting abort reference useless.

With that in mind, and in the spirit of your comment about offering a static, would it make sense to propose the following?

1. Provide a static method on AbortSignal for creating [signal, abort function] tuples. The goal being to allow incantations like

const {signal, abort} = AbortSignal.create()

2. Deprecate or eliminate the AbortController interface altogether (due to it being a redundant and flawed construct for it's intended purpose)

[annevk]

I think with 2 you are overstating your case as there is a reason to separate these two, though perhaps the revealing constructor pattern could have been used instead. I forgot how we ended up with the current design.

[bathos]

flawed construct for it's intended purpose

The API seems to cover more than one use case and control flow pattern. Separation of the controller and signal allows sharing the signal with code that supports reacting to the abort event / observing the aborted status even when that code shouldn't also have control over whether or when the signal aborts. In cases where the consumer of the signal should be able to control aborting, well - that's when you pass the controller instead of the signal alone.

We use AbortController/AbortSignal a ton but it's very rare that we pass controllers around. I only turned up one instance of sharing a controller just now out of hundreds of usages of AC/AS. So while we likely have different most-common-needs, it does not seem accurate to me to say the current model is flawed: it meets our needs and your suggested change would not.

I agree that utilities for common AC/AS usage patterns would be fantastic though - to me it seems that's the underlying issue, maybe? AC/AS provide the control flow primitives, but the API surface isn't specifically optimized for any of them in particular, so e.g. in your case it ends up seeming overly complex, while in our case it tends to demand a lot of explicit wiring-up. At this point we have a pretty extensive set of helper functions for working with signals (e.g. await race(signal, promise) potentially rejects w/ AbortError DOMException) and I suspect many of them are things people end up reinventing over and over right now.

---

Edit: I was incorrect when I said that your last suggested alternative wouldn't meet our needs. I misread it and thought you wanted to see abort() as a method of the signal itself, so the above text reflects that incorrect reading in various ways. A free floating abort() would work just as well for retaining the separation and is no better or worse for us, though naturally I would hope an API upon which pretty much all the code I've written in the last three years relies would not be suddenly removed even if a new variant were introduced :)

[annevk]

Anything we do will have to be backwards compatible. I vaguely wonder if we should add this:

const signal = new AbortSignal(abort => {
  $('#someButton').addEventListener('click', abort);
});

@jakearchibald @domenic do you remember why we went with an explicit AbortController object?

[jakearchibald]

@broofa https://developers.google.com/web/updates/2017/09/abortable-fetch#the_history some details on the history here, including links to the original GitHub discussions where folks pushed really heavily for the separation.

[jakearchibald]

Making the suggested change would allow for this code, instead...

const {signal, abort} = new AbortController();

// delegate abort
$('#someButton').addEventListener('click', abort);

This is an antipattern. It means abort is being called with an Event object. controller.abort() currently accepts zero arguments, but that might change in future. For example, it might become controller.abort(reason), and that might cause an unexpected change to code like above. I wrote about this antipattern here https://jakearchibald.com/2021/function-callback-risks/.

As for the revealing constructor pattern, I don't think it works outside of the most basic code examples.

Let's make the "abort button" example a bit more realistic:

async function fetchAndDisplay() {
  const controller = new AbortController();
  const abortClick = () => controller.abort();
  stopButton.addEventListener('click', abortClick);

  try {
    const response = await fetch(url, { signal: controller.signal });
    await displayResult(response);
  } finally {
    stopButton.removeEventListener('click', abortClick);
  }
}

To avoid leaks, the abort button listener is removed on job completion, failure, or abort. Now let's try with a revealing constructor:

async function fetchAndDisplay() {
  const signal = new AbortSignal(abort => {
    stopButton.addEventListener('click', () => abort());
  });

  try {
    const response = await fetch(url, { signal });
    await displayResult(response);
  } finally {
    stopButton.removeEventListener('click', /* ah bollocks */);
  }
}

We can't remove the listener in this case, because it isn't in scope. You can work around this by complicating the code:

function fetchAndDisplay() {
  const job = Promise.resolve().then(async () => {
    const response = await fetch(url, { signal });
    await displayResult(response);
  });

  const signal = new AbortSignal(abort => {
    const abortClick = () => abort(); 
    stopButton.addEventListener('click', abortClick);
    job.finally(() => stopButton.removeEventListener('click', abortClick));
  });

  return job;
}

This maintains the revealing constructor pattern, but now the code is split between two places, rather than having a linear flow. There are also crossed dependencies; the 'job' needs the signal to pass to fetch, but the signal needs the 'job' to know when the job settles. This is why the job needs to use Promise.resolve() to wait for a microtask.

Another way to solve this is to break out of the revealing constructor:

async function fetchAndDisplay() {
  let abortClick;
  const signal = new AbortSignal(abort => {
    abortClick = () => abort();
    stopButton.addEventListener('click', abortClick);
  });

  try {
    const response = await fetch(url, { signal });
    await displayResult(response);
  } finally {
    stopButton.removeEventListener('click', abortClick);
  }
}

But it doesn't feel like we're getting much benefit over the API we have now.

An abort button is the simple case. Another common pattern is "abort the current operation in favour of a new operation". With the current API:

let currentJob = Promise.resolve();
let currentController;

async function showSearchResults(input) {
  if (currentController) currentController.abort();
  currentController = new AbortController();

  return currentJob = currentJob.catch(() => {}).then(async () => {
    const response = await fetch(getSearchUrl(input), { signal: currentController.signal });
    await displayResult(response);
  });
}

I can't think of a way to make this fit the revealing constructor pattern, so again I find myself just passing it outside of the constructor:

let currentJob = Promise.resolve();
let currentAbort;

async function showSearchResults(input) {
  if (currentAbort) currentAbort();
  const signal = new AbortSignal(a => currentAbort = a);

  return currentJob = currentJob.catch(() => {}).then(async () => {
    const response = await fetch(getSearchUrl(input), { signal });
    await displayResult(response);
  });
}

[annevk]

Thanks for writing that up @jakearchibald! Do you find you need to "break out" with the Promise constructor as well? Anyway, it seems like more justification is needed here before considering new wrappers of sorts.

[jakearchibald]

Do you find you need to "break out" with the Promise constructor as well?

Very occasionally, but not often. The last time I remember doing it was to do the "abort the current operation in favour of a new operation" pattern before we had AbortController:

let currentJob = Promise.resolve();
let currentReject;

function showSearchResults(input) {
  if (currentReject) currentReject(new DOMException('', 'AbortError'));
  const abortPromise = new Promise((_, reject) => currentReject = reject);

  return currentJob = currentJob.catch(() => {}).then(() => {
    return Promise.race([
      abortPromise,
      Promise.resolve().then(async () => {
        // …do stuff…
      }),
    ]);
  });
}

But I've since rewritten that code to just use AbortController.

[broofa]

First, let me apologize for my "flawed" comment (😳). That came across a bit sharper than intended. And I'll be the first to acknowledge that the {signal, abort} tuple I've proposed is basically the same thing as AC, the only difference being that it doesn't carry the weighty formality of a class, and has abort well and truly bound to signal. So it's not like I'm offering a complete redesign here. Anyhow... sorry about that.

details on the history here

@jakearchibald : Thanks for the link to the previous discussion(s). I was sure such discussion existed but wasn't sure where. I'm afraid I don't have time to read through that in detail at the moment, but I'll try to catch up on it later (maybe this weekend?)

This is an antipattern. It means abort is being called with an Event object

Having been bitten by Firefox's lateness argument back in the day, I appreciate this concern. I question whether this is something WHATWG should be policing, though. If a user is concerned with this, () => abort() works.

I think @bathos has the right path forward here. Treat AC/AS as the control flow primitives and build utilities on top of that.

[jakearchibald]

I'm really interested in ways we can make this stuff easier to use btw! I'm still keen on the second idea in #946 (comment), but it seems that conversation fizzled out.

If a user is concerned with this, () => abort() works.

But now we're talking about a big API change just to make it () => abort() rather than () => controller.abort(), and I don't see a ton of value there.

[benjamingr]

I just want to add I agree with Domenic's and Anne's sentiments here regarding it being weird if only AbortController had a bound abort method.

I also want to add that creating AbortControllers in code is a lot more rare (once, when flows start and once when flows fork) than passing AbortSignals in (which happens very often).

[Jamesernator]

Do you find you need to "break out" with the Promise constructor as well?

I've personally used it quite a few times in particular for concurrency management, e.g. locks, (async) task queues, etc etc. So it isn't something that never gets in the way.

A typical example would be something like lock:

class Deferred {
  resolve;
  reject;
  promise = new Promise((resolve, reject) => {
    this.resolve = resolve;
    this.reject = reject;
  });
}

class Lock {
  #locked = false;
  #pendingRequests = [];
  
  #release = () => {
    assert(this.#locked);
    const firstPending = this.#pendingRequests.shift();
    if (firstPending) {
      firstPending.resolve(once(this.#release));
    } else {
      this.#locked = false;
    }
  }
  
  async acquire() {
    if (this.#locked) {
      const deferred = new Deferred();
      this.#pendingRequests.push(deferred);
      return await deferred.promise;
    }
    this.#locked = true;
    return once(this.#release);
  }
}

---

I think controller.abort() is generally nicer when working with abort, because as mentioned, abort tends to happen at the same level of creation (rather than inside the constructor init function like most promises). (Although I definitely wouldn't say no to const { signal, abort } = AbortSignal.create(), as the AbortController does seem kinda redundant just for one method).

Although I can say one place I have specifically wanted the revealing constructor pattern on AbortSignal is to be able to subclass AbortSignal, which is otherwise undoable as AbortSignal has to be tied to a controller (so super() always throws, as new AbortSignal() always throws). It might be doable by some awful super override hacks, but real subclassing would be a lot nicer.

Although I wouldn't even need the subclass (actually a parallel implementation + extra methods) if a few suggestions were all added, the only reason I have the parallel implementation is that the listed issues are basically all things I use heavily when working with AbortSignal so it's just too cumbersome not to have them consistently available.

[broofa]

[Closing out issues I've authored that appear to be stagnant.]