search

clj-commons / manifold

A compatibility layer for event-driven abstractions
1.02k stars 106 forks source link

Remove timeout future execution if deferred completes before timeout #160

Closed tanzoniteblack closed 5 years ago

tanzoniteblack commented 5 years ago

Make sure that once a deferred that has had a timeout attached to it via deferred/timeout! completes, that the scheduled timeout function is removed from the execution scheduler and that the reference to the original deferred is removed to allow for garbage collection.

Solves: https://github.com/ztellman/manifold/issues/159

tanzoniteblack commented 5 years ago

I've verified that this actually does what I think it does by doing memory profiling with YourKit.

ztellman commented 5 years ago

The idiomatic way to "cancel" something in Manifold is to put a value into a deferred before the computation it represents can complete or even begin. So instead of having a separate in-cancel function, I'd like to just extend in to notice when the deferred it returns has been coopted.

tanzoniteblack commented 5 years ago

@ztellman updated the PR to have the callback to cancel the execution done if the deferred that manifold.time/in returns has been coopted. I agree that this definitely makes sense & fits with the paradigm of this library. If chain isn't the preferred way to do this, let me know and I'll update again.

tanzoniteblack commented 5 years ago

Also: Here's the memory profiling without this change being used: without-callback-cancel

And with this change: with-callback-cancel

tanzoniteblack commented 5 years ago

@ztellman bumping this PR back to your attention. We've been using it at Yummly for the last 2 months for our main API and it's been rock solid so far

ztellman commented 5 years ago

Apologies for the delay, merging now.

tanzoniteblack commented 5 years ago

@ztellman do you have a planned timeline on cutting a release with this change? We're currently running in production with a release of my branch, but it'd be nice to get back on mainline.