-
Notifications
You must be signed in to change notification settings - Fork 10
Fix rabbitio out panic 'send on closed channel' #38
base: master
Are you sure you want to change the base?
Conversation
@@ -53,7 +53,8 @@ var outCmd = &cobra.Command{ | |||
signal.Notify(c, os.Interrupt, syscall.SIGTERM) | |||
go func() { | |||
<-c | |||
fmt.Println(" Interruption, saving last memory bits..") | |||
fmt.Println("Interruption, saving last memory bits..") | |||
rabbit.SafeStop() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so this is about graceful shutdown on sigterm right? What's the consequence of not doing the SafeStop? Some stale channel on the broker?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. It provides a graceful pause of AMQP-messages consuming before the closing go-channel, which writes messages on disk. The consequence of not doing the SafeStop is panic: send on closed channel on SIGTERM.
if err != nil { | ||
log.Fatalf("rabbit channel cancel failed %s", err) | ||
} else { | ||
for _ = range time.Tick(2 * time.Second) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do we need the time tick here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a "crutch", to allow for all AMQP-messages to be put into the write channel before it closes. I'm not so familiar with the golang, to avoid time tick/sleep usage, but it looks like a better approach exists.
r.tag, // consumer | ||
false, // noAck | ||
false, // exclusive | ||
false, // noLocal | ||
false, // noWait | ||
nil, // arguments | ||
nil, // args |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to change these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you're right.
#36