AbortSignal listeners should be attached with { once: true} to avoid memory leaks

[paulbrimicombe]

According to the NodeJS documentation:

Any event listeners attached to the AbortSignal should use the { once: true } option (or, if using the EventEmitter APIs to attach a listener, use the once() method) to ensure that the event listener is removed as soon as the 'abort' event is handled. Failure to do so may result in memory leaks.

A recent change to @smithy/node-http-handler has introduced event listeners being added to AbortSignals without { once: true }.

Even though I've not found any evidence of memory leaks, we should probably follow the advice in the NodeJS documentation.

[nicholasgriffintn]

I've also raised a similar issue for this here: https://github.com/smithy-lang/smithy-typescript/issues/1318

[paulbrimicombe]

I've also raised an issue for this here: #1318

This is a slightly different issue @nicholasgriffintn (but I was inspired by your issue!).

1. If you expect a new AbortSignal to be passed on every request, you should use { once: true } as per the NodeJS docs, but do not have to remove the event listener once the request is completed.
2. If you expect re-use of AbortSignal objects across requests, you should use { once: true } as per the NodeJS docs, and must also remove event listeners once requests have completed.

As you can see, both situations require the change for this issue (and the associated PR), but more work is needed if AbortSignals are to be re-used across multiple requests.

Currently it seems that the expected pattern in this library is that you pass a different AbortSignal on each request (which is a perfectly valid API, though it's not documented anywhere that I've found in the @smithy docs). I've checked and AbortSignal objects are garbage collected once the reference to them is removed, regardless of whether they have event listeners attached so you shouldn't need to worry about memory leaks unless you're hanging on to references to the AbortSignal.

I actually think re-using AbortSignal objects across multiple requests can be problematic since you lose fine-grained control over the cancellation (since requests can overlap / happen in parallel).

[nicholasgriffintn]

Yeah I was pretty sure we'd need both.

That all makes sense, for our use case, we're aborting globally so do have a reference used across multiple requests, which used to work fine and is also something I do with libraries like undici.

I think your pr may be a solution to some of this problem for sure.

In unici, they are also returning a callback to remove the listener, that seems to be all that needs to be done to make abort controllers work globally.

[kuhe]

The changes from https://github.com/smithy-lang/smithy-typescript/pull/1332 were released in

https://www.npmjs.com/package/@smithy/node-http-handler/v/3.1.2 https://www.npmjs.com/package/@smithy/fetch-http-handler/v/3.2.1

For AWS SDK and other client consumers, you can remove your lockfile for a fresh install that should bring these in transitively.

[kuhe]

pass a different AbortSignal on each request

We assumed this was the MO, but that was wrong given the issue demonstrated here. Thanks for reporting and contributing the fix.