Improve InterruptedException handling in com.eventstore.dbclient.GrpcClient.

[jezovuk]

Current implementation of com.eventstore.dbclient.GrpcClient.pushMsg(Msg) logs and swallows the InterruptedException. It seems more appropriate to propagate the exception instead (perhaps wrapped in RuntimeException or similar), since this is called on application/caller thread.

There might be other places (in GrpcClient or elsewhere) which could benefit from stricter interrupt handling. (I have checked GrpcClient code but do not have sufficient understanding of client lifecycle to detect any further problems).

[YoEight]

Hey @jezovuk ,

That exception will ever occur if the thread got interrupted while we are waiting for msg to be inserted in the queue. In other word, the user has to interrupt the thread they are running on themselves. I argue there is no value at propagating that exception upward for this reason specifically.

[jezovuk]

Technically, not only if interrupted while waiting, but also if interrupted before attempting the insert (before this.messages.put(msg) is called).

Regarding this: In other word, the user has to interrupt the thread they are running on themselves.

My scenario is as follows:

• I have a 'background service/module' with single-threaded executor running on a schedule (via org.springframework.scheduling.concurrent.ThreadPoolTaskScheduler)
• Each execution (task's run() implementation) checks external data source via HTTP/REST call, and if data changes are detected appends new event to ES via call to com.eventstore.dbclient.EventStoreDBClient.appendToStream(...)
• After calling appendToStream(...) our code calls join() on returned CompletableFuture (in order to make sure event is indeed appended before moving on with additional logic)
• If/when we want to stop this module (we have dozens of similar modules like this in single process/app and we sometimes need to start/stop some of them explicitly), we invoke shutdown() on mentioned ThreadPoolTaskScheduler
• This in turn leads to Spring code invoking shutdownNow() on underlying java.util.concurrent.ExecutorService
• ExecutorService.shutdownNow() will interrupt the running thread (single thread executor in our case; note that this interrupt is NOT initiated from executor thread itself, but from thread handling the explicit 'stop' request - JMX operation handling thread in our case)
• If this stop/interrupt happens before event append operation is called/initiated (and no other operation handles the interrupt before append), com.eventstore.dbclient.GrpcClient.pushMsg(Msg) will catch, log and 'eat' the InterruptedException
• Executor thread will forever remain stuck waiting on CompletableFuture.join(), without any notification (an error will be logged, but the thread/code will not be aware of this, obviously)
• ThreadPoolTaskScheduler shutdown will eventually time out waiting for underlying executor to stop, but the executor thread will remain stuck forever

If com.eventstore.dbclient.GrpcClient.pushMsg(Msg) were to propagate the interrupt (re-throw InterruptedException), we could/would handle it in our task impl (run() method) and skip any further operations in that run, thus enabling quick and orderly executor (and thus ThreadPoolTaskScheduler and entire module) shutdown.

[YoEight]

Technically, not only if interrupted while waiting, but also if interrupted before attempting the insert (before this.messages.put(msg) is called).

The docs are pretty clear on this, it won't happen prior to that point. Also, comment out that put call and there is no InterruptedException to catch.

I do agree though that your scenario warrants propagating that exception. However, I don't see any way to achieve this without breaking the API, I don't think wrapping that exception into a RuntimeException to be satisfactory.

Unless you already have an implementation on your mind, I will need some time to rewrite that part of the code to support your use case.

[jezovuk]

The docs are pretty clear on this, it won't happen prior to that point. Also, comment out that put call and there is no InterruptedException to catch.

Perhaps I was not precise enough. When I said "if interrupted before attempting the insert", I was talking about Thread.interrput() being called (from another thread) on Thread object representing the trhead pushing the message before it invokes this.messages.put(msg) (or even before it invokes com.eventstore.dbclient.GrpcClient.pushMsg(Msg)). It is clear that actual InterruptedException can only be raised by pushMsg(...) here.

As for proposed solution, I have no problems with wrapping the InterruptedException in RuntimeException (or some more specific subclass of it), at least as a starting point, until API changes required to propagate InterruptedException directly can be thought through and implemented. It is certainly better for me than current behavior which can lead to indefinite blocking on CompletableFuture.join().