stan.go icon indicating copy to clipboard operation
stan.go copied to clipboard

Is `PublishAsync` non blocking all the time?

Open huangjunwen opened this issue 7 years ago • 5 comments

I was assuming PublishAsync is non-blocking. But read this in publishAsync:

// Use the buffered channel to control the number of outstanding acks.
pac <- struct{}{}

Then there is chance PublishAysnc being blocked. I would suggest changing to die fast here:

select {
  case pac <- struct{}{}:
  default:
    // Some cleanup
    return ErrMaxInFlight
}

Application can choose to publish again or just abandon.

huangjunwen avatar Nov 10 '18 03:11 huangjunwen

This is well documented here. We could add an option to fail on reaching MaxPubAcksInfligh value, but as of now, the code behaves as intended.

kozlovic avatar Nov 10 '18 16:11 kozlovic

Thanks for your reply. The main problem we face is that the publish API lacks of "cancellation" support (e.g. context cancellation). In fact, it is most desirable if the interface is like:

Publish(ctx context.Context, subject string, data []byte) error
PublishAsync(ctx context.Context, subject string, data []byte, ah AckHandler) (string, error) 

Then blocking or not doesn't matter since it can wait for the context and the pubAckChan at the same time:

select {
  case pac <- struct{}{}:
  case <-ctx.Done():
    // Some cleanup
    return ctx.Err()
}

But this would break compatible (or add a new interface?). So i think another easier way is to die fast when reaching MaxPubAcksInflight to make it non blocking.

Otherwise under current API, if i want to precisely control cancellation of publishing, I have to allocate another go routine per call. This is quite expensive.

huangjunwen avatar Nov 10 '18 23:11 huangjunwen

Yes, adding a connection option that instruct to fail-fast when publish async would block is best in that it does not break backward compatibility. Does that sound reasonable?

kozlovic avatar Nov 10 '18 23:11 kozlovic

Yes, i think this is reasonable :-)

huangjunwen avatar Nov 10 '18 23:11 huangjunwen

...but supporting a Context might be the more idiomatic Go solution.

JensRantil avatar Apr 09 '19 09:04 JensRantil