Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove timeout future execution if deferred completes before timeout #160

Merged

Conversation

tanzoniteblack
Copy link
Contributor

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: #159

@tanzoniteblack
Copy link
Contributor Author

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

@ztellman
Copy link
Collaborator

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.

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: clj-commons#159
@tanzoniteblack tanzoniteblack force-pushed the remove-timeout-callback-upon-success branch from 3270d6d to fcb244e Compare November 12, 2018 16:39
@tanzoniteblack
Copy link
Contributor Author

@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
Copy link
Contributor Author

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

And with this change:
with-callback-cancel

@tanzoniteblack
Copy link
Contributor Author

@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
Copy link
Collaborator

ztellman commented Feb 1, 2019

Apologies for the delay, merging now.

@ztellman ztellman merged commit 5d986a5 into clj-commons:master Feb 1, 2019
@tanzoniteblack
Copy link
Contributor Author

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants