Fix goroutines in chan-throttler* logic

[dparis]

The two goroutines in chan-throttler* were set up to loop infinitely, assuming that the loop would park on the bucket channel put/take operations. Since the bucket channel never closes even if the throttled input channel does, these goroutines will stay parked (and thus not garbage-collected) forever.

I made some minor changes to the looping logic to address this. Now, both loops within the go blocks only occur when the bucket channel is open. Once the throttler input channel is closed, along with closing the output channel, the bucket channel is now closed too. This stops both loops within the go blocks which allows them to return and eventually be cleaned up.

I also replaced the pipe macro invocation, and since it wasn't being called anywhere else, removed the macro. If you'd like, I can put it back.

All of the midje tests pass. If there's further testing you'd like me do, let me know.

Cheers!

[brunoV]

Thank you for your contributions, @dparis. I actually had spotted this and fixed it locally but life got the best of me and I never pushed the change. I'll accept this pull request after we go through my comments above.

[dparis]

Should have a new commit ready soon. Thanks for the review.

[dparis]

Reverted the pipe macro and updated the logic accordingly. All tests pass cleanly.

As a side note, I found a post from @halgari on the clojure mailing list suggesting that parked goroutines with no remaining references can be GC'ed: https://groups.google.com/d/msg/clojure/_KzEoq0XcHQ/W3V7uhJHPOwJ

Could be this fix is moot if that is the case.

In looking at all this, though, I did find something more concerning. When using fn-throttler and throttle-fn, a chan-throttler is created and closed over but the input channel is never exposed. As such, the input channel can never be closed and the bucket filling goroutine will continue to loop forever. I confirmed this by adding a println to the bucket filling loop, creating and binding a fn-throttler in the REPL, and then rebinding that var to something else. If I understand correctly, that should release the only reference to the function throttler. Ideally this would cause the goroutines to stop looping, but I continued to see the println output.

If these loops don't ever halt, even if the function closing over them goes out of scope, I could see this becoming a problem if one were to create many throttled functions or channels dynamically. Since my current app may need to do that in a long-running context, I might just inline a basic throttling implementation.

I'm still getting used to core.async, so if I've misunderstood anything, please let me know. I'd be happy to be wrong!

[dparis]

Whoops, I missed your reply regarding the macro. I'll re-revert the pipe macro change. :grin:

[brunoV]

Great stuff, @dparis! I'll check out your branch to take it for a spin tonight and then merge it to master. The pull request looks good.

Let me address your comments:

Could be this fix is moot if that is the case.

Could be. I noticed he published a tutorial on how GC works for channels, but I haven't watched it yet. In any case, I think it's good to close a channel if we know it's not going to be used anymore, so I'd say we keep your change.

And your observation about the non-GCable channel in throttle-fn is correct. I thought that it wouldn't generally matter since there'd be a limited number of throttled functions in an application defined at compile time. But yeah, if you are going to create an unbounded number of throttled functions dynamically, then it could potentially become a problem.

In that case, there are two options. Either we add a way to "undeclare" a throttled function, or we just recommend the usage of the throttle-chan API for this case.

I'd love to hear your input on that! And again, thanks for the contribution and the great input.

[brunoV]

Merged!