Check if channel is nil before updating it

[julienschmidt]

In observed the following panic in the wild:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x54 pc=0x104cf3c]

goroutine 144 [running]:
github.com/rabbitmq/amqp091-go.(*Channel).setClosed(...)
	/home/runner/go/pkg/mod/github.com/rabbitmq/[email protected]/channel.go:94
github.com/rabbitmq/amqp091-go.updateChannel(...)
	/home/runner/go/pkg/mod/github.com/rabbitmq/[email protected]/types.go:329
github.com/rabbitmq/amqp091-go.(*Connection).dispatchN(0xc000b2ea00, {0x1d5c150, 0xc00096f1b8?})
	/home/runner/go/pkg/mod/github.com/rabbitmq/[email protected]/connection.go:539 +0xbc
github.com/rabbitmq/amqp091-go.(*Connection).demux(0xc00039cf28?, {0x1d5c150, 0xc00096f1b8})
	/home/runner/go/pkg/mod/github.com/rabbitmq/[email protected]/connection.go:500 +0x5b
github.com/rabbitmq/amqp091-go.(*Connection).reader(0xc000b2ea00, {0x1d57540?, 0xc0004620c8?})
	/home/runner/go/pkg/mod/github.com/rabbitmq/[email protected]/connection.go:600 +0x23d
created by github.com/rabbitmq/amqp091-go.Open
	/home/runner/go/pkg/mod/github.com/rabbitmq/[email protected]/connection.go:265 +0x34c

Trying to understand what happens, it turns out the nil pointer deference here https://github.com/rabbitmq/amqp091-go/blob/v1.5.0/channel.go#L94 actually seems the be caused by the map access here https://github.com/rabbitmq/amqp091-go/blob/v1.5.0/connection.go#L538, which returns nil if the key doesn't exist / the map is empty.
Thus, a check whether the channel is nil is missing.

[lukebakken]

Thanks for your submission.

If channel is nil at this spot, it seems to be indicative of a bug somewhere else that I would like to track down, rather than just checking for nil.

Is there a reliable way to reproduce this scenario?

[julienschmidt]

I need to investigate a bit further, but telling from the logs, the following might have happened:

1. An exchange that did not exist on the vhost was accessed, causing an exception: Exception (404) Reason: "NOT_FOUND - no exchange 'XXX' in vhost '/'"
2. The wrapper library (https://github.com/go-redis/redis https://github.com/wagslane/go-rabbitmq) reconnected (new connection)
3. Right after establishing the new connection, it received frames it was not quite prepared for?

[michaelklishin]

I doubt that https://github.com/go-redis/redis is a wrapper for this library. In addition, the 404 exception in your example would not close the connection as it is a channel exception. No new frames would be sent on this channel but it could that there were some in flight.

So the improvement would be: drop frames for channels that are not currently open (that the client knows of) with a warning, several other clients do something like that.

[julienschmidt]

Sorry, copied the wrong link 🤦‍♂️
https://github.com/wagslane/go-rabbitmq is the wrapper library

[lukebakken]

@julienschmidt thanks again for your pull request. Considering that the channel is checked for nil soon after, it's reasonable to also check where you indicate. I've added @michaelklishin's suggestion to log dropped frames.