Link detach does not set closed to true

[vcabbage]

Hi! I maintain an AMQP 1.0 client for Go (https://github.com/vcabbage/amqp). While investigating vcabbage/amqp#60 I noticed that closing a link was hanging.

It seems when the client send it's detach, with Closed=true, RabbitMQ responds with a detach with Closed=false. (Technically it's sending it with a null value for Closed, but the default is false.)

To my knowledge, non-closing detaches are used to detach from a link and reattach later. I'm not sure that's something RabbitMQ supports (my client doesn't). In any case, I believe it would be correct for RabbitMQ to respond with a closing detach.

The spec also says:

Note that one peer MAY send a closing detach while its partner is sending a non-closing detach. In this case, the partner MUST signal that it has closed the link by reattaching and then sending a closing detach.

Do you agree with this assessment? I can provide debug logs and/or packet captures if it would be helpful.

Thanks!

[michaelklishin]

Can you please provide a small example we can run with go run to reproduce?

[kjnilsson]

Looking at the code RabbitMQ replies without even inspecting the value of the incoming closed flag. Link recovery isn't supported by the plugin so we should probably always return closed=true for all detaches assuming this is allowed by the protocol.

[vcabbage]

@michaelklishin Sure thing. Here's a minimal example:

package main

import (
	"log"

	"pack.ag/amqp"
)

func main() {
	// Create client
	client, err := amqp.Dial("amqp://guest:guest@localhost:5672/")
	if err != nil {
		log.Fatal(err)
	}

	// Open a session
	session, err := client.NewSession()
	if err != nil {
		log.Fatal("Creating AMQP session:", err)
	}

	// Create link
	sender, err := session.NewSender(
		amqp.LinkTargetAddress("/queue"),
	)
	if err != nil {
		log.Fatal("Creating sender link:", err)
	}

	// Close link
	sender.Close()
}

Running as is it should just hang.

If you run it with go run -tags debug it should print out something like:

TX: Begin{RemoteChannel: 0, NextOutgoingID: 0, IncomingWindow: 5000, OutgoingWindow: 4294967295, HandleMax: 0, OfferedCapabilities: [], DesiredCapabilities: [], Properties: map[]}
RX: Begin{RemoteChannel: 0, NextOutgoingID: 0, IncomingWindow: 65535, OutgoingWindow: 65535, HandleMax: 4294967295, OfferedCapabilities: [], DesiredCapabilities: [], Properties: map[]}
TX: Attach{Name: pqaJaBRgAvzLXqzRrrUIYvaIujDpHYjxeUBrVfdw, Handle: 0, Role: Sender, SenderSettleMode: <nil>, ReceiverSettleMode: <nil>, Source: <nil>, Target: source{Address: /queue, Durable: 0, ExpiryPolicy: , Timeout: 0, Dynamic: false, DynamicNodeProperties: map[], Capabilities: []}, Unsettled: map[], IncompleteUnsettled: false, InitialDeliveryCount: 0, MaxMessageSize: 9223372036854775807, OfferedCapabilities: [], DesiredCapabilities: [], Properties: map[]}
RX(Session): Attach{Name: pqaJaBRgAvzLXqzRrrUIYvaIujDpHYjxeUBrVfdw, Handle: 0, Role: Receiver, SenderSettleMode: <nil>, ReceiverSettleMode: <nil>, Source: <nil>, Target: source{Address: /queue, Durable: 0, ExpiryPolicy: session-end, Timeout: 0, Dynamic: false, DynamicNodeProperties: map[], Capabilities: []}, Unsettled: map[], IncompleteUnsettled: false, InitialDeliveryCount: 0, MaxMessageSize: 0, OfferedCapabilities: [], DesiredCapabilities: [], Properties: map[]}
RX(Session): Flow{NextIncomingID: 0, IncomingWindow: 65535, NextOutgoingID: 0, OutgoingWindow: 65535, Handle: 0, DeliveryCount: <nil>, LinkCredit: 65536, Available: <nil>, Drain: false, Echo: false, Properties: map[]}
TX(Session): Detach{Handle: 0, Closed: true, Error: *Error(nil)}
RX(Session): Detach{Handle: 0, Closed: false, Error: *Error(nil)}

[kjnilsson]

That the amqp 1.0 plugin returns with closed=undefined (false) is established. The question here is what is the right thing to do given link recovery isn't supported.

Ideally we'd only support link close frames with closed=true and error on anything else. Then we can reply with closed=true and all is well, protocol-wise. This could potentially break some clients if they don't always set the closed field to true and may be a bit too strict?

Alternatively we could always return closed=true (whatever the input frame has set). If the client sent closed=false and then got closed=true back from the plugin it would then try to re-establish the link at which point it will get an error (as link recovery isn't supported).

[michaelklishin]

@kjnilsson I think returning an error is the right thing to do for now.

[grs]

I would argue do the simplest thing and just echo back what the peer initiating the deatch sent as the close flag. Detaching without close=true doesn't imply that the link can be resumed as the terminus can be deleted when detached.

[ansd]

Fixed in RabbitMQ 4.0.