Buffer is leaking a thread every time it's full

[fbernier]

I recently deployed an update of this gem and immediately noticed the thread count per instance was constantly increasing over time.

Here's what it looked like:

After looking into the code, I found the cause.

When the buffer is initialized, it calls reset_buffer to set up the buffer state. In there, it launches a timer thread containing an infinite loop.

on buffer_receive, the default behaviour when the buffer is full is to drop the messages. It does so by calling... reset_buffer, which ends up creating a new thread while the other still lives.

The result is a new thread that never exits for every full buffer.

[fbernier]

To be honest, this whole buffer implementation seems super dangerous to me.

Using a multiple producer, single consumer ring buffer without any locks at all sounds much better and safer than a bunch of mutexes.

For example, the buffer_full method is most likely also racey.

This catch-all rescue is also pretty bad.

[dwbutler]

This is a similar issue to #97 where each call to reset_buffer creates new mutexes. I think the correct fix here is the same: create a single, memoized flushing thread that is reused when the buffer state is reset.

I'm not a huge fan of the buffering implementation either. I agree that it is potentially racy and the mutexes are dangerous.

A ring buffer actually sounds like the perfect data structure to use here. It would also have the advantage of losing less of the buffer when the buffer gets full.

[fbernier]

Seems like all the building blocks for a proper buffer implementation are available here: https://github.com/ruby-concurrency/concurrent-ruby/tree/master/lib/concurrent/channel/buffer

and the documentation being here: https://github.com/ruby-concurrency/concurrent-ruby/blob/master/doc/channel.md

I might eventually do it unless you feel like it!

[dwbutler]

The fix is released in 0.19.2.

Thanks for the awesome suggestion! I'll be reading the docs very carefully. I've actually considered using Concurrent Ruby Channels before. They didn't appear to be suitable at first because they don't support removing a batch of items, and putting them back in if something went wrong. But now that I've thought about it some more, I think the requirements can be satisfied by having two channels and two consumers:

• A pending channel stores incoming log messages.
• One consumer takes messages out of the pending channel and puts them in the flush channel.
• Another consumer takes messages out of the flush channel and attempts to write them (in batch) to the output. If it fails, it keeps retrying.

I might be overthinking it...