Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Is PublishAsync non blocking all the time? #210

Open
huangjunwen opened this issue Nov 10, 2018 · 5 comments
Open

Is PublishAsync non blocking all the time? #210

huangjunwen opened this issue Nov 10, 2018 · 5 comments
Assignees

Comments

@huangjunwen
Copy link

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.

@kozlovic
Copy link
Member

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.

@huangjunwen
Copy link
Author

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.

@kozlovic
Copy link
Member

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?

@huangjunwen
Copy link
Author

Yes, i think this is reasonable :-)

@JensRantil
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants