Fix for pending takes not cleaned up when they timeout

[KingMob]

About

Fixes #194 by cleaning the pending put/take lists every X calls

Changes

• Periodically cleans up pending takes/puts, and removes any that have been realized (presumably due to timeout)
• Fixes value returned when the number of takes exceeds the limit, to match the documentation
• Adds tests
• Defines vars for the put/take limits and how often to check, in case anybody wants to tune those

Internal details

Producer and Consumer were changed from types to records. We need to access the deferred field in both, but that incurs a reflection penalty in the cleanup fn. The choice was between making a duplicate cleanup fn, adding a protocol to access .-deferred and hinting with that, or switching to records and using keyword access.

I'd rather not make a duplicate fn that's 99% the same, and a macro feels like a bit much just for this. I checked with Criterium, and keyword access is only 25% slower (ditto for a new protocol) but still just a few nanoseconds, so this seems acceptable.

[KingMob]

For some reason, I can't choose @kachayev as a reviewer. @slipset do you know what that's about?

Also Erik, you added CircleCI support, so I'm going to remove the Travis support unless I hear someone say otherwise; it seems to be failing. I'll add support for more JDKs later.

[slipset]

Should be fine to remove travis. I've also invited @kachayev as a maintainer of manifold

[wdullaer]

Would it be possible to make the max-dirty-takes/max-dirty-puts configurable? I have a usecase where I have very little puts and takes, but it is important that there are no timed out takes at all still in the queue. Right now I need to send dummy messages on the stream to ensure the dirty takes are cleaned out.

[KingMob]

@wdullaer I'm a bit unclear as to what you're asking here.

For starters, max-dirty-takes/max-dirty-puts can already be reconfigured with alter-var-root and constantly to a different value.

Backing up a bit, what problem are you running into that you're trying to prevent timed-out takes? Couldn't you use take! over try-take! for that?

[wdullaer]

I forgot about alter-var-root, that would indeed work.

My issue is a bit involved: I have some code that polls a web endpoint, and returns a function to cancel this polling to the caller. In my initial implementation, I had the cancel function set a deferred to success and used d/alt in my loop. Unfortunately this ties the lifetime of all the http deferreds with the cancel deferred together. Because the cancel deferred typically lives for the entire program, it causes a big memory leak.

My solution is to have the cancel function pop a message on a stream, and do a take! in each loop. The deferred from the take! can then be put in the alt. At the end of the loop, I can set this take! deferred to success and have it pruned from the pending consumers on the stream. It's important those completed deferreds are pruned right away, because the polling can take some time to reach 64 iterations (it's an event loop, sometimes there's a lot, sometimes there's none).

Right now, I hack around this by putting dummy messages on the stream to realize the take! deferred, but it's a bit ugly.

[KingMob]

@wdullaer Hmm. I'm guessing your original solution used alt in a loop to choose between the next http polling stream value and the cancellation deferred? Yeah, that would tie the alt deferreds to the cancellation deferred through the on-realized callbacks. I would have thought the http deferreds would clean up once they resolve, though. Of course, the alt deferreds would usually hold the http deferreds value, so that's still a big leak if they don't get cleaned up.

The best solution would be to cancel the callbacks on the unresolved deferreds, which should be a matter of switching from on-realized to using add-listener/cancel-listener. However, not all of them are currently cancellable. If it's a Future or an IPending, the "callbacks" are actually tasks submitted to an Executor that block on the realized value of the Future. Maybe that's not a problem, even if it's not a universal solution. Will have to think about this more. Would you be willing to write up an issue with some example code?

For your particular use case, instead of a stream that you continually take from and have to fill with dummy values, what about this? Use a single cancellation deferred and each loop, check to see if it's been realized. If realized, exit. If not realized, continue with the loop. To avoid blocking on the http deferreds if the loop's been canceled, use alt with a timeout on the http deferreds. This ensures it'll still check for cancellation, at the cost of a bit of CPU cycles.

[KingMob]

@ztellman @kachayev Do either of you want/have time to comment on this? If not, that's ok, I'll invite some of the other people who expressed interest in maintaining Manifold to take a look.

[ztellman]

Hi, sorry for not getting to this sooner. This looks good, but it's at least theoretically possible for us to hit the dirty-* threshold concurrently on two separate threads, so it would be safest to put a (locking l ...) around cleanup-expired-deferreds.