jetstream: ordered consumer: handle closed subscriptions gracefully

[zonque]

In orderedConsumer.reset(), retryWithBackoff() is returned from with false, nil in case the subscription is closed. In this case, var cons Consumer is still nil, and the code crashes in the attempt to cast it to a pullConsumer later on.

Fix this by catching the nil case explicitly.

Signed-off-by: Daniel Mack [email protected]

[zonque]

Closes #1656

[zonque]

This is a regression from 66b92a9 it appears.

[zonque]

@piotrpio ping

[Jarema]

@zonque sorry for the delay.
I wonder - are you able to reproduce it?
If yes - can we have a test for it?

[zonque]

It's tricky to test because the issue is triggered by a race condition between the creation of a consumer and the teardown of a NATS server. Not sure how this could be tested, but this is very clearly a bug, as you can easily identify the code path that leads to a nil pointer dereference 🤷

[piotrpio]

Hello @zonque, thanks for finding this and sorry it tool so long to review.

Your solution solves the nil pointer, but I have a few suggestions on how it can be improved:

1. Don't use nats.ErrSubscriptionClosed - this is a core NATS error implicating that a NATS subscription was closed - in the context of JetStream consumers it does not make much sense.
2. I don't think just returning an error is enough - the way it works after your change it will trigger an error callback for Consume(), which I don't think should be done since the consuming was stopped by the user (either with Stop() or Drain()). Similarly, for Messages(), you'll get an ambiguous nats.ErrSubscriptionClosed when calling Next().

My suggestion would be to:

1. introduce a new error variable, e.g. errOrderedConsumerStopped (similarly to errOrderedSequenceMismatch) which can be internal, we don't have to expose it to the user. We can return this error if cons == nil.
2. Handle the error properly in Consume() and Next(). For Consume(), I think we can simply return (here), for Next(), we can return ErrMsgIteratorClosed since we're sure there will be no more messages (here and here)

Again, sorry it took that long. Let me know if you'll be working on that or I should take it off your hands.

[zonque]

Again, sorry it took that long. Let me know if you'll be working on that or I should take it off your hands.

Thanks for the feedback. As you have a better view on the bigger picture, I think it might in fact be easier if you roll your own version of the fix?

[piotrpio]

@zonque, sure, I'll take care of this and make sure the fix is included in the next release.

[piotrpio]

I created a PR with the fix: #1686

So I'll be closing this one - thanks for your contribution!