Calling Channel() on an empty connection panics

[jrubinator]

Calling Channel() on an uninitialized connection panics, rather than return an error.

Code:

package main

import (
	"fmt"

	amqp "github.com/rabbitmq/amqp091-go"
)

func main() {
	// panics instead of errors
	conn := &amqp.Connection{}
	_, err := conn.Channel()
	fmt.Println(err.Error())
}

Expected Behavior: prints out an error

Actual Behavior: panics

Invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x8 pc=0x4eec5d]

goroutine 1 [running]:
github.com/rabbitmq/amqp091-go.(*allocator).next(0x0)
	/tmp/gopath3681466631/pkg/mod/github.com/rabbitmq/amqp091-go@v1.5.0/allocator.go:72 +0x1d
github.com/rabbitmq/amqp091-go.(*Connection).allocateChannel(0xc000002140)
	/tmp/gopath3681466631/pkg/mod/github.com/rabbitmq/amqp091-go@v1.5.0/connection.go:686 +0xd2
github.com/rabbitmq/amqp091-go.(*Connection).openChannel(0x405a19?)
	/tmp/gopath3681466631/pkg/mod/github.com/rabbitmq/amqp091-go@v1.5.0/connection.go:709 +0x25
github.com/rabbitmq/amqp091-go.(*Connection).Channel(...)
	/tmp/gopath3681466631/pkg/mod/github.com/rabbitmq/amqp091-go@v1.5.0/connection.go:736
main.main()
	/tmp/sandbox2002244425/prog.go:14 +0x2b

See https://go.dev/play/p/sseC5Beiapd

[lukebakken]

Please feel free to submit a pull request to address this issue.

[Zerpet]

The root cause is our interpretation of "closed" in Connection.closed field. 0 means false, 1 means true; therefore, the zero-value of Connection, i.e. closed = 0, means the connection is "open".

I don't think it's worth it changing the meaning of this field in all the codebase. amqp.Connection struct does not have a usable zero-value, and it has Dial* functions to initialise the struct properly.

[jrubinator]

I'd be happy to attempt a PR, if I can get some help figuring out the right place to check for this is (and what to check).

One place would be around https://github.com/rabbitmq/amqp091-go/blob/v1.5.0/connection.go#L686, where we could notice that allocator is nil, and return an error. I was kind of hoping there'd be something more direct, but @Zerpet's comment makes me wonder.

Is allocator a reasonable thing to check there?

@Zerpet - you might be interested in why I'm doing this. I'm trying to test some behavior when an application receives an invalid (eg. closed) connection, but during my tests I don't currently have an available MQ instance to test on. So that amqp.Connection isn't usable was something of the point, until it panic'ed. I'm very open to suggestions on how to do this better (mocking or otherwise)

[Zerpet]

Hey, I came back from holidays today and wanted to acknowledge your last message. Bear with me while I get back to speed. The topic on how to mock/fake a connection has come up a few times, and there was a concrete way, despite the lack of interfaces in this library.

[Zerpet]

@jrubinator this comment streadway/amqp#164 (comment) from the original author of this library summarises how to unit test parts of this library. In your concrete case, you may need a connection factory, that returns an interface that you can mock.

With regards to the panic, I'd like to keep this behaviour, as it is a mistake to use a zero-value Connection. This is aligned to Go standard library, where a mistake (e.g. closing a closed channel) causes a panic.

[jrubinator]

Thanks! Re. the mocking, I'm not sure that solves my problem. I'll try not to get too into the weeds, here's a shot:

We have a library that wraps Connection:

type OurConnection struct {
    *amqp.Connection
}

and we expect to have other developers call eg. ourConnection.Channel(). Those developers can definitely mock using the strategy you linked, but I was hoping that we could offer a Quality-of-Life option like being able to set OurConnection.MockPublish = true and have Publish() calls originating from that connection append to somewhere that the developer's test could read from.

I don't think it's the end of the world if we can't; just sharing my use-case. Let me know if you want any more details.

And Re. the panicking: thanks for the consideration!

Happy new year!

[Zerpet]

Thanks for the context. I think you may need to abstract even further the AMQP primitives to accomplish what you described. I'm guessing that OurConnection.Channel() returns an *amqp.Channel, and then the developer can do Channel.Publish(). You could abstract/wrap Publish, perhaps to receive a pointer to a channel as parameter, and decide whether to do real publish or fake publish, based on your boolean.

Re. the panic, I appreciate that panicking is not nice, however, I'm unsure that checking for nil allocator and returning an error will resolve your issue. We would have to return a specific error like ErrBadState or similar, and, if I understood correctly, you'd like to test on a "closed" connection i.e. Channel() returns ErrClosed. You'd probably want to differentiate between different errors e.g. ErrChannelMax vs ErrClosed, so it is not enough to return any error from Connection.Channel(). I don't want to return ErrClosed when allocator is nil, because, in zero-value state, Connection.IsClosed() returns false, which contradicts the error from Connection.Channel().

Edit: Happy new year! ⭐

[jrubinator]

I'm guessing that OurConnection.Channel() returns an *amqp.Channel, and then the developer can do Channel.Publish().

Precisely! Yeah, I think what you describe is indeed what we'd have to do. (They can also call non-Publish methods, which is why I was hoping for something magical here :))

if I understood correctly, you'd like to test on a "closed" connection

I was actually looking to test on a subset of "closed" connections - just those that have never been open, so maybe a new error ErrNeverOpened? If I squint, I can see it without seeing a contradiction (it's not closed, but it's definitely not open either). But I imagine that's nowhere in the MQ specs, so totally understand not wanting to implement that kind of thing here.

[lukebakken]

I don't think it's worth it changing the meaning of this field in all the codebase. amqp.Connection struct does not have a usable zero-value, and it has Dial* functions to initialise the struct properly.

I agree with @Zerpet. Thanks for all of the interesting discussion.