search

connor4312 / cockatiel

🐩 A resilience and fault-handling library. Supports Backoffs, Retries, Circuit Breakers, Timeouts, Bulkhead Isolation, and Fallbacks.
MIT License
1.59k stars 50 forks source link

fix: prevent abort listener leaks in TimeoutPolicy #82

Closed biggyspender closed 4 months ago

biggyspender commented 1 year ago

re: https://github.com/connor4312/cockatiel/issues/81

ghost91- commented 4 months ago

@connor4312 any chance of this being merged?

We are currently considering to start using this library, and it looks extremely great in general (thanks for your work!), but this might be a blocker.

It seems like the fix is pretty simple and straight forward. Is there anything holding it back?

ghost91- commented 4 months ago

With this change, is it even necessary to keep the WeakRef, or could we just reference the abort controller directly?

As far as I understand, the point of using the WeakRef is that when the abort controller controller goes out of scope, it can be cleaned up by the garbage collector even when the parent signal is never canceled and stays in scope.

With with these changes, wouldn’t the controller be eventually cleaned up when either the callback finishes regularly, or we run into the timeout? Unless the callback never returns or the timeout is pretty long, this should prevent memory leaks, right?

I'm mostly asking because I would also like to use this library in browsers and WeakRef is a relatively recent addition. It is not supported by all browsers I need to support, and unfortunately, it’s also not possible to polyfill it (since it directly integrates with the GC). You could only create a sham implementation that is actually just a strong ref in the end.

connor4312 commented 4 months ago

Thanks for the PR! The test is handy, and the overall approach is good but I wanted to implement it in a slightly different way and did so in 3d63ca173f2469f1193831366565cff1acb597f0. I really appreciate you taking the time to report the issue and create a repro and fix! This will be published shortly.

biggyspender commented 2 weeks ago

@connor4312 Coming back to this a few months later! There were def a few shortcomings to my fix. Thanks for taking the time to deepdive into this and providing your own fix. I'll probably need to use cockatiel in a new product soon, so I'll get the chance to have another play soon.