Panic in producer's router about go-nsq HOT 2 CLOSED

Plus 1

from go-nsq.

I'm not too surprised there's a bug in this part of the codebase, it's not heavily used by the original developers. I think the original devs mostly either publish only to localhost on each server that produces messages, in some cases with this library but in other cases just using http from a service written in a different language, or through an http load balancer to a small cluster of nsqd (thus via http). The Consumer code is much more heavily used.

I don't think gotos, goroutines, or races are involved in this. Each Producer starts one goroutine, running Producer.router(), which runs for the life of that Producer. (One more short-lived goroutine is started in Producer.close() when the producer is done.) The two goto you can see in router() are exactly equivalent of a break to label of the outer for (and the Producer exits at that point, and can no longer hit this panic).

Only the single router() goroutine started by a particular Producer will append or pop from the transactions slice, there is no concurrent access.

The bug might simply be that this code assumes that onConnError() can only be called in relation to an in-flight transaction. But if it is called when zero transactions are in flight, you'll get this panic. It actually looks like that shouldn't happen, it should only be called for an error reply message from the server, which should correspond to a sent message ... but of course it should be robust against seeming to get bytes from the server that look like some kind of reply when there are is no transaction in flight (and then close the Producer because message send/recv is out of sync).

I'll open a quick PR. If you can provide a reproducer script, or validate that the PR makes a noticeable difference in your infrastructure (confirming the hypothesis), that would be helpful. Here it is: #346

from go-nsq.