stan StreamingConnection.publish() hangs forever in some cases

[tul]

We've hit a problem a couple of times recently where our servers have become permanently blocked when trying to publish to STAN.

Specifically, publishes are hanging when trying to put in the BlockingQueue which is utilised to control the number of outstanding acks:

StreamingConnectionImpl.java:348

       // Use the buffered channel to control the number of outstanding acks.
        try {
            pac.put(PubAck.getDefaultInstance());  // <<--- hang
        } catch (InterruptedException e) {
            // TODO:  Reevaluate this.
            // Eat this because you can't really do anything with it
        }

If we tear down the STAN connection by completely disconnecting and reconnecting from the server, these threads still remain blocked in the publish on the now closed connection.

I'm thinking perhaps this put() should be an offer() with a timeout, allowing publishes to optionally fail back to the caller (via a thrown exception) if too many are in flight.

Alternatively, or perhaps also, when a streaming connection is torn down, perhaps it should empty the BlockingQueue so that any publishes hanging on the put/offer would at least progress.

[tul]

I have made some fixes in a fork of this lib:

1. When closing a streaming connection, clear the pubAcks channel, unblocking any publishes that were waiting on it, and allowing their threads to progress.

2. You can now set an optional "publish timeout" in the options when making your streaming connection. If publish() has to block for longer than this (due to max inflight limit) it will throw a TimeoutException

3. Within publish(), after putting into the acks channel, use a cached NATS connection instead of the member, to handle the connection having been closed whilst you were blocking. Also, if the connection has been closed (meaning the NATS Connection.publish() will throw), make sure to remove the ack from the in flights channel before rethrowing.

I'm not familiar (yet) with Mockito and so I haven't made any unit tests for these fixes yet, but am happy to do so if you want to proceed with this patch.

The changes are in PR 80

[tul]

On a related note, we encountered an issue on the code without my patch which might be worth mentioning.

When a STAN publish() is not acknowledged in time by the server, it will call the user supplied ack handler to let the user know the publish failed. If that callback directly calls publish() (e.g. to resubmit the lost message) then it has the potential to cause an implicit deadlock in the system.

This happens, as if the new call to publish() blocks due to too many in flight requests, it has blocked the Timer thread which is responsible for removing the timed out acks. If acks are not being removed any other way (due to a network connectivity issue meaning a bunch of NATS messages were lost / discarded for example), then the client library has become locked and any future publish() attempts will block those calling threads permanently too.

My proposed patch offers a work around by adding an optional timeout to the publish() call, but for cases where no time out is being used, there is still a vulnerability.

[sasbury]

Can you test your patch on 2.0.1, i tried to do minimal work to get us on the new nats client, so i doubt i fixed this issue. Leaving the issue open for testing/work.

[sasbury]

Closing for now, please reopen if you reproduce on the latest version.