search

wagslane / go-rabbitmq

A wrapper of streadway/amqp that provides reconnection logic and sane defaults
https://blog.boot.dev/golang/connecting-to-rabbitmq-in-golang-easy/
MIT License
773 stars 126 forks source link

panic: close of closed channel #28

Closed rzajac closed 3 years ago

rzajac commented 3 years ago

Not sure how this might happen but I got:

panic: close of closed channel

goroutine 15 [running]:
github.com/wagslane/go-rabbitmq.(*channelManager).startNotifyCancelOrClosed(0xc0000c4840)
        /var/lib/jenkins/go/pkg/mod/github.com/wagslane/go-rabbitmq@v0.6.2-0.20210618171750-0f20affc2859/channel.go:85 +0x1c5
created by github.com/wagslane/go-rabbitmq.newChannelManager
        /var/lib/jenkins/go/pkg/mod/github.com/wagslane/go-rabbitmq@v0.6.2-0.20210618171750-0f20affc2859/channel.go:35 +0x21a

It is probably happening because channel was closed inside NotifyCancel method https://github.com/wagslane/go-rabbitmq/blob/0f20affc2859ccb8250fd0380994c324ef2c287f/channel.go#L59

victorges commented 3 years ago

So I was building a new AMQP producer application. I originally went for implementing all the connection handling logic on top of streadway/amqp, but after seeing how much complexity I'd need to maintain I decided to take another look at this lib.

Currently this issue is what's preventing me from using it though, as the system I'm working on cannot have panics in production as I don't control all deployments.

I think the correct fix here would be to not close the notification channels, since when we call Notify* on the *amqp.Channel it actually takes ownership of the channel and necessarily closes it on shutdown. There's also no problem leaving a channel open indefinitely, only if there are receivers blocked on it (if there's a sender blocked and we close the channel they will panic anyway, so not much we can do).

So all we need is to remove the code that closes those channels and add a minimum buffer of 1 to them , as suggested in some amqp examples as well. WDYT?

wagslane commented 3 years ago

I'm very sorry I don't have time this week to look at it, maybe next week? That said, always feel free to open a PR as I can review code faster than writing it.

On Mon, Jul 12, 2021, 5:15 PM Victor Elias @.***> wrote:

So I was building a new AMQP producer application. I originally went for implementing all the connection handling logic on top of streadway/amqp, but after seeing how much complexity I'd need to maintain I decided to take another look at this lib.

Currently this issue is what's preventing me from using it though, as the system I'm working on cannot have panics in production as I don't control all deployments.

I think the correct fix here would be to not close the notification channels, since when we call Notify on the amqp.Channel it actually takes ownership of the channel and necessarily closes it on shutdown. There's also no problem leaving a channel open indefinitely, only if there are receivers blocked on it (if there's a sender blocked and we close the channel they will panic anyway, so not much we can do).

So all we need is to remove the code that closes those channels and add a minimum buffer of 1 to them , as suggested in some amqp examples as well. WDYT?

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/wagslane/go-rabbitmq/issues/28#issuecomment-878658980, or unsubscribe https://github.com/notifications/unsubscribe-auth/AFHZKVNLNF2TMGTCTDOTQ43TXNZPVANCNFSM5ACUVCLA .

victorges commented 3 years ago

@wagslane done! :) https://github.com/wagslane/go-rabbitmq/pull/29