Re: [whatwg/fetch] Proposal: fetch with multiple AbortSignals (#905)

> fwiw, it can be simplified further:
> 
> ```ts
> function anySignal(signals: AbortSignal[]): AbortSignal {
>   const controller = new AbortController();
> 
>   for (const signal of signals) {
>     if (signal.aborted) return signal;
> 
>     signal.addEventListener("abort", () => controller.abort(signal.reason), {
>       signal: controller.signal,
>     });
>   }
> 
>   return controller.signal;
> }
> ```
> 
> …now that signals can be used to remove event listeners.

@jakearchibald I'm afraid there is a bug in your implementation. When you do: `if (signal.aborted) return signal;` you are not unsubscribing listeners added in previous loop iterations, so they continue to listen for events. As @pauldraper has mentioned, we need to additionally call `controller.abort()` in this case as well to unsubscribe prior listeners.

So the final code should look like this:

```ts
function anySignal(signals: AbortSignal[]): AbortSignal {
  const controller = new AbortController();

  for (const signal of signals) {
    if (signal.aborted) {
      controller.abort();
      return signal;
    }

    signal.addEventListener("abort", () => controller.abort(signal.reason), {
      signal: controller.signal,
    });
 }

  return controller.signal;
}
```

By the way, there is no need to pass `signal.reason` as @pauldraper did, because this signal is not returned from the function in the first place due to the early exit and is solely used to trigger listeners deregistration.

@jakearchibald could you please update your example to fix this? I'm afraid people from Google search could accidentally copy it with the memory leak.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/whatwg/fetch/issues/905#issuecomment-1816547024
You are receiving this because you are subscribed to this thread.

Message ID: <whatwg/fetch/issues/905/1816547024@github.com>