feat: Add clear method for a composite signal

[ukstv]

Not quite optimal, and I would prefer it all to be TypeScript code, yet here it goes. At least, as a provisionary PR open to critique.

Now anySignal function returns AbortSignal with a twist. If you call signal.clear(), it clears all the event handlers set up on the constituent AbortSignals. An example in #23 could be modified to not cause any memory leaks:

class Loader {
  shutdownController = new AbortController()

  async load(id: string, options: { signal: AbortSignal }) {
    const compositeSignal = anySignal([this.shutdownController.signal, options.signal])
    try {
      await doSomething(compositeSignal)
      return "Success!"
    } finally {
      compositeSignal.clear()
    }
  }
}

[ukstv]

@jacobheun Hey, any critique on the PR?

Let me share rationale for the change. In here, for example: https://github.com/libp2p/js-libp2p-delegated-content-routing/blob/master/src/index.ts#L200 as well as in other js-libp2p code you see that there is a permanent AbortController. On every function call it gets wrapped with any-signal. In a happy scenario, that leads to +1 event listener to AbortController.signal. There are two implications of that:

1. Slow unbounded memory growth: every event listener gets forever attached to the permanent AbortController.signal.
2. User-level warnings. AbortController.signal is yet another EventEmitter, which complains after certain number of listeners got attached to it. Plenty of js-libp2p code use setMaxListeners like on https://github.com/libp2p/js-libp2p/blob/master/src/peer-routing.ts#L114 . Usually that eliminates user-level warnings indeed, yet it requires some discipline to add that function to an appropriate signal: https://github.com/libp2p/js-libp2p-delegated-content-routing/blob/master/src/index.ts#L200 for example.

If something like explicit clear is provided, it would be easier to the library user not to make mistakes when handling AbortSignals.

[avious00]

@jacobheun

@jacobheun Hey, any critique on the PR?

Let me share rationale for the change. In here, for example: https://github.com/libp2p/js-libp2p-delegated-content-routing/blob/master/src/index.ts#L200 as well as in other js-libp2p code you see that there is a permanent AbortController. On every function call it gets wrapped with any-signal. In a happy scenario, that leads to +1 event listener to AbortController.signal. There are two implications of that:

1. Slow unbounded memory growth: every event listener gets forever attached to the permanent AbortController.signal.
2. User-level warnings. AbortController.signal is yet another EventEmitter, which complains after certain number of listeners got attached to it. Plenty of js-libp2p code use setMaxListeners like on https://github.com/libp2p/js-libp2p/blob/master/src/peer-routing.ts#L114 . Usually that eliminates user-level warnings indeed, yet it requires some discipline to add that function to an appropriate signal: https://github.com/libp2p/js-libp2p-delegated-content-routing/blob/master/src/index.ts#L200 for example.

If something like explicit clear is provided, it would be easier to the library user not to make mistakes when handling AbortSignals.

Hey @jacobheun - I'm Avi, PM at Ceramic. Wanted to expand on Sergey's 2nd point on user level warnings, we've seen this become a developer experience pain point - several forum and discord posts on this, e.g. [1] [2] [3]

thanks in advance for juggling this on top of your EM work, appreciate you.

[jacobheun]

@ukstv thanks for the nudge, I missed the PR as I was out on holiday in March. Have you tested this fix against js-libp2p to verify it fixes the problem there? I'll take a look at this in the mean time

[jacobheun]

Not quite optimal, and I would prefer it all to be TypeScript code

If you want to convert it to TS I don't have a problem with that. Overall adding clear seems to be the cleanest way of handling this given there's no real terminal state for an AbortSignal if it's never aborted.

If this gets you what you need, I'm fine with the changes as is and can ship a major release (given the types have changed).

[achingbrain]

It's funny, I just came across this exact same problem.

[ukstv]

If you want to convert it to TS I don't have a problem with that.

I have not really grokked how aegir works. What if I create a fully-functional repo with the code that I see is the most optimal route (breaking changes included, as Proxy has some perf penalty), and you transplant TS and tests into this very well-known package?

The breaking change I mentioned is that maybe the return type of anySignal function should be [signal, clear]. BTW, as @achingbrain you are here, would you consider it a problem if the return type changes like that?

[achingbrain]

I have a small preference for returning a single value rather than a tuple as to me at least if feels a bit more idiomatic for js.

If we're looking at this with fresh eyes, perhaps this module would return an extended AbortController, since that's where methods like .abort live, it seems like a reasonable place to put a clear method?

We already do things like add extra methods to timeout-abort-controller so there's some precedent there.

I am pro converting to TypeScript in general but I'd not want to block fixing this bug on that (or the above tbh), it's a bit of a show-stopper.

[ukstv]

If we're looking at this with fresh eyes, perhaps this module would return an extended AbortController, since that's where methods like .abort live, it seems like a reasonable place to put a clear method?

Here my understanding of idiomatic JS says clear belongs to a signal. Usually we pass around AbortSignal instances, rather than AbortController instances.

I am pro converting to TypeScript in general but I'd not want to block fixing this bug on that (or the above tbh), it's a bit of a show-stopper.

For sure!! I just wanted to use a nice coincidence that we have smart people here participating in the discussion. Moving to TS, changing the interface are all topics adjacent but not blocking to this PR. All of that can be done separately.

[achingbrain]

Here my understanding of idiomatic JS says clear belongs to a signal.

My thinking was that a function taking an AbortSignal usually listens for it's abort event or checks if it has been aborted already and that's all.

.clear is accountancy that's necessary around the signal, rather than it being part of the signal itself, so it's up to the function that created the composite signal to decide whether to abort the operation and/or trigger any cleanup that's necessary so the natural place for it seemed to be on a controller.

Usually we pass around AbortSignal instances, rather than AbortController instances.

To be clear, my intention was not to pass controller instances outside of the calling function, more like:

const controller = anySignal([...signals])

try {
  return await onwardFunction(args, {
    signal: controller.signal
  })
} finally {
  controller.clear()
}

Not sure if any-signal is a good name for a module with the above behaviour but there you go.

[jacobheun]

Since we have enough of an agreement on this, I'm going to merge this, update the readme and cut a major release. Happy to continue conversations on future changes, but want to make sure folks aren't blocked on this.

[jacobheun]

Published in 4.0.0

[avious00]

Since we have enough of an agreement on this, I'm going to merge this, update the readme and cut a major release. Happy to continue conversations on future changes, but want to make sure folks aren't blocked on this.

thanks Jacob and Alex!