Bounded queues for the EventHandler thread

[thomaslee]

The unbounded queue fronting the 'event' thread can cause trouble when the
EventHandler is unable to keep up with the workload. This can lead to heap
exhaustion, GC issues and failure modes that are generally considered "bad".

Band-aid over this with a semaphore to limit the number of tasks in the queue.
The semaphore is opt-in and disabled by default to avoid any nasty surprises
for folks upgrading.

Also add 'EventSource.awaitClosed()' to allow users to wait for underlying
thread pools to completely shut down. We can't know if it's safe to clean
up resources used by the EventHandler thread if we can't be certain that it
has completely terminated.

[bwoskow-ld]

Hi @thomaslee,

Thank you for the proposed contribution, and sorry for taking so long to get to this!

As with any concurrency issue, we need to spend some time closely reviewing the code to understand and approve what is changing.

To help with that matter, can you further explain under what conditions you're running into heap exhaustion, GC issues, and other "bad" failure modes? Are you using this library in conjunction with a LaunchDarkly SDK or as a direct dependency for a different use case?

Cheers,
@bwoskow-ld

Filed internally as 110323.

[thomaslee]

Hey @bwoskow-ld, no worries.

As with any concurrency issue, we need to spend some time closely reviewing the code to understand and approve what is changing.

Of course. 🙂

To help with that matter, can you further explain under what conditions you're running into heap exhaustion, GC issues, and other "bad" failure modes?

Sure. Basically the issue occurs when the EventHandler implementation is unable to process events at a rate exceeding the event arrival rate. To put it another way: the issue occurs if you're unable to process events faster than they arrive.

In my case it was under very heavy load during some local testing: my CPU-bound EventHandler was unable to keep up with the rate of incoming events. More moderate load in the face of simple backpressure on the thread running the EventHandler could lead to similar issues. For example, if we're writing to a database in our EventHandler and there's some kind of temporary outage in the database server. In either scenario the eventExecutor queue can back up until you run out of memory (or GC gets so bad that the service is having a very bad time regardless.)

Should be easy-ish to reproduce with a reasonable amount of throughput and a "slow" EventHandler implementation so that the rate of arrival of inbound events drastically outpaces the processing rate. Thread.sleep ought to do the trick! You'll want to push it hard enough that you're not waiting around all day if you're looking to repro the GC issue though.

My change here just introduces a(n optional) semaphore to enforce a limit on the number of in-flight tasks queued up for eventExecutor effectively bounding the troublesome queue. You might prefer to introduce a blocking queue or something like that instead of the semaphore, this was just the smallest/safest change I could think to make without breaking too many interfaces. 🙂

Are you using this library in conjunction with a LaunchDarkly SDK or as a direct dependency for a different use case?

I was using it as a direct dependency, not familiar with the SDK.

[eli-darkly]

Sorry this PR has been languishing. The change looks reasonable and I particularly appreciate the unit test. I'll be merging this into a temporary branch rather than master for now, because some updates may be needed due to changes in the package implementation, but if there are no surprise problems it should be in the next release.

[thomaslee]

@eli-darkly not at all, happy to see this land. Thank you!

[eli-darkly]

This is included in the 2.5.0 release.

[thomaslee]

Fantastic, thank you!