Allow JetStream Publish retries iff ErrNoResponders was returned.

[derekcollison]

This is to avoid small blips from leader changes surfacing to the end application.

Signed-off-by: Derek Collison derek@nats.io

[coveralls]

Coverage decreased (-0.2%) to 85.097% when pulling 2a5ee5f on js_pub_retry_2 into c92df80 on main.

[kozlovic]

This test had 2 failures out of the 4 runs, so may want to see if that can be improved.

[kozlovic]

Note that the call below uses the context that is created at the beginning, so it is likely that the deadline is the original one, whereas here you make each request with the original timeout, so maybe that should be reduced by the time already spent? If not, there will be some difference in behavior between context/timeout.

[wallyqs]

I think this might be fine, I like how the context controls the max duration of the retries logic. For example:

ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()
_, err := js.Publish(subj, msg, nats.Context(ctx), nats.RetryWait(500*time.Millisecond), nats.RetryAttempts(30))

Above would mean to attempt to publish for 10 seconds max, although the retries would potentially take 15s but the context still sets the deadline. Also means that possible to have infinite retries, so would retry as needed until the context expires (edit: actually not possible to do infinite retries with current logic so made a suggestion)

[derekcollison]

I will do a rough estimation here subtracting out o.rwait and bailing if local ttl in the loop goes negative. Will post separate CL for ease of review but can squash before merging.

[wallyqs]

Suggested change

	for r := 0; err == ErrNoResponders && r < o.rnum; r++ {
	for r := 0; err == ErrNoResponders && (r < o.rnum || o.rnum < 0); r++ {

That change would make it possible to allow infinite retries, in order to let retries happen as needed while context is still active:

ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
defer cancel()
_, err := js.Publish(subj, msg, nats.Context(ctx), nats.RetryWait(500*time.Millisecond), nats.RetryAttempts(-1))

[derekcollison]

Should we make sure a ctx is present to do that one?
I will make the change but we could get into an infinite loop if the stream is truly gone and their is no context provided to the Publish call.

[derekcollison]

Change made below.

[kozlovic]

LGTM

[wallyqs]

LGTM!