vcabbage / amqp

AMQP 1.0 client library for Go.
https://godoc.org/pack.ag/amqp
MIT License
134 stars 96 forks source link

Asynchronous Send mode. #179

Open alanconway opened 4 years ago

alanconway commented 4 years ago

The current Sender.Send() blocks for the remote disposition. This is fine for many applications, but it does not allow sending at full network throughput because it forces the sender t idle for at least 2x the network latency between each send.

The doc mentions using goroutines, but this is not really workable - you lose control of message ordering. What you really want is an ordered stream of messages going out and an independent ordered stream of dispositions coming in.

One solution is to send the dispositions back on a channel, like https://github.com/apache/qpid-proton/blob/go1/electron/sender.go#L61

However I am currently leaning towards a design like this:

Sender {  AsyncSend(Message) Result }
Result { Wait() error }

For the sync case this is almost as easy to use as blocking Send():

    err := AsyncSend(Message).Wait()

and it supports the the channel solution for async:

    type myResult struct { r: Result; extra: ...}
    myResults := make(chan myResult, n)
    go func() { ... ; myResults <- myResult{r: s.AsyncSend(m), extra: ...} ...}() 
    go func() { for r := range(myResults) { if err := r.r.Wait() {boom()} else { proces(r.extra)}})()

I'd like to hear comments on the design before I submit a PR.

vcabbage commented 4 years ago

Makes sense and I think the API you propose is overall reasonable.

Some minor thoughts/questions:

alanconway commented 4 years ago

Makes sense and I think the API you propose is overall reasonable.

Some minor thoughts/questions:

  • Method should be SendAsync so that it's ordered next to Send in Godoc. +1

  • I'm wondering whether SendAsync should return any errors that happen before transmitting. This would remove the possibility of chaining, but I don't think that's important to preserve. Thoughts?

I have thought about this mucho :) A single way to handle errors makes a simpler API, but there's also a correctness argument: you might be tempted to think that a direct error return from Send() guarantees that the message was not sent. This is a dangerous assumption, an error return from net.Conn.Write() does not guarantee that part (or all) of the buffer wasn't received remotely and the failure happened before the ack got back. So unless all layers of the stack are coded very carefully to differentiate "definitely didn't happen" from "maybe didn't happen" it's much safer to assume "maybe". You have to write the "maybe" code anyway and it's semantically correct even if the failure was definite.

  • Result may be a bit generic. Perhaps SendAsyncResult or SendResult? I don't think either is great, but the name probably isn't all that important as long as it's reasonably descriptive.

+1 I used "Outcome" in electron but that is equally random. "Settlement" might be a better AMQP-esque term. We can use this to provide more info about the AMQP settlement so folks can handle reject, modify and the like if they want (or ignore them if not and just take the Error())

I defer to your judgement, whatever you feel is in keeping with the project style.

vcabbage commented 4 years ago

SGTM. For the result type let's go with SendResult.