- From: Benjamin Gruenbaum <notifications@github.com>
- Date: Thu, 04 Feb 2021 06:30:21 -0800
- To: whatwg/dom <dom@noreply.github.com>
- Cc: Subscribed <subscribed@noreply.github.com>
- Message-ID: <whatwg/dom/issues/946@github.com>
Hey, Node.js AbortController integration is working great - almost all Node.js APIs now take a `signal` parameter to allow cancellation. ### The Problem I've spoken to a bunch of library authors regarding AbortSignal ergonomics and one feedback that has been brought up a bunch is that the following pattern is very common and rather easy to get wrong: ```js if (signal.aborted) { // cleanup } else { const listener = () => resource.cleanup(); signal.addEventListener('abort', listener); resource.on('close', () => signal.removeEventListener(listener)); } // Happy to point to 8+ places in the Node.js code base we have this sort of code ``` The issue is that this is pretty easy to get wrong and cause memory leaks because: - You can forget to remove the listener correctly when the resource is finished and leak memory. - You can forget to check the signal is aborted ahead of time and deal with both cases. One alternative suggested was to have a utility method that helps with this, the above code bemes: ```js // the second parameter is so the listener is removed when the resource gets GCd. aborted(signal, resource).then(() => resource.cleanup()); // just returns a regular promise aborted(signal); ``` It was also brought up this has several other use cases like `Promise.race([somePromiseICantCancel, aborted(signal)])` which is very nifty because it integrates AbortSignal with promise combinators without requiring JS spec changes. Ref: https://github.com/nodejs/node/issues/37220 and https://github.com/getify/CAF/issues/21 ### Idea It would be very cool if instead of Node.js shipping this as a utility this could be part of the specification (a method on AbortSignal). That way we have a consistent cross-platform way to handle the use case. ### Doubts I am still not entirely convinced this is what we should do (either for Node or WHATWG). I think this is pretty nifty but I wanted to ask (the nice) people here if there are any alternatives (both for the Node.js utility and for the spec). Additionally, I wanted to know if browsers think this is even an issue and find this interesting. I think this is important since it makes comosing cancellation APIs without bugs easier. *Regardless of browser interest being a prerequisite for spec inclusion it would be very interesting to me as information to decide what route to pursue in Node.js*. ### Alternatives I know I wrote this above as well but I am very interested in knowing if other people thought about this problem as well and have different ideas about how to solve it. ### Implementation I am happy to contribute the needed spec changes to the WHATWG spec if you are willing to put up with the low quality of my work. The feedback provided (by Domenic and Anne) last time was very helpful and I learned a bunch. (I am starting a new job soon in a company who is a member of WHATWG and I'll need to re-sign the license I think) cc helpful people who helped with the ideas last time I worked on related stuff @domenic @annevk @jakearchibald cc library authors I talked to about cancellation and brought up use-cases that encouraged this pattern @bterlson @benlesh @getify ------ Note 1: I am putting on my Node.js hat because that's the one I put on when I talked to users - but to be fair/honest the majority of use cases people brought up were browser API based and not Node.js. Note 2: As mentioned in the `.follow` issue I am still very much "on it" and in fact this utility could significantly simplify that problem (hopefully!). Note 3: I am in _no rush_, I am hopeful that this will be slightly less mind-melting than that `.follow` stuff :D -- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/whatwg/dom/issues/946
Received on Thursday, 4 February 2021 14:30:34 UTC