search

clj-commons / manifold

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

deferred/timeout! holds onto reference of deferred until timeout has expired, even if future is completed #159

Closed tanzoniteblack closed 5 years ago

tanzoniteblack commented 5 years ago

We recently tried using manifold to make our API at Yummly fully async, but what we discovered was that using a deferred/future with deferred/timeout! caused the future to not be able to be garbage collected until the timeout period had expired. In all situations where our API is performing as expected, since the timeouts are generally only for bad cases, this causes us to create deferred values faster than they're garbage collected, until eventually we end up having memory issues under peak load.

I'll be looking into a way to fix the timeout! function to cancel the scheduled execution if the deferred value is finished before the timeout, but would be happy to receive your input prior to be submitting a PR.

ztellman commented 5 years ago

This will require changing the API of manifold.time.IClock so that in returns a cancel function, and then using that within timeout!. These are both reasonable changes, so I'm happy to either take a PR or get around to it myself, if you're okay to wait.

tanzoniteblack commented 5 years ago

That's the same conclusion I'd come to on how to approach the problem. It's an important improvement for Yummly to be able to adopt this library in our main API, so I'll go ahead and start working on it (because hey, getting paid to improve opensource libraries is a good thing).

tanzoniteblack commented 5 years ago

Resolved with PR 160