nsqd: io.EOF when client-conn is closed

[mreiferson]

NSQd does not distinguish io.EOF.
nsq/nsqd/protocol_v2.go

        line, err = client.Reader.ReadSlice('\n')
        if err != nil {
            if atomic.LoadInt32(&client.State) == stateClosing {
                err = nil 
            } else {
                err = fmt.Errorf("failed to read command - %s", err)
            }   
            break
        }

[mreiferson]

hmmm, I suppose we could consider io.EOF as a benign error (that the other side had closed the connection cleanly)?

thoughts @jehiah

[jehiah]

Well, we do ignore io.EOF when a client sends a CLS command (if i'm reading this right).

I think that's important because it stops the message flow to allow for a clean close. It's unclear what our expected clean close steps are for producer connections. I'd be happy to begin expecting a CLS msg there as well because it's explicit.

[mreiferson]

Circling back to this...

Agreed about the general value of CLS in the consumer case. I don't think there's anything specific about producer vs. consumer here, though. I think @fjjiaboming was just pointing out that we could conceivably not log io.EOF errors under the assumption that they're TCP-level "clean" closes.

[chaosue]

producer won't send a "SUB" command before "CLS", then the error "cannot CLS in current state" will be emitted, while the consumer it to send the "SUB" command before "CLS", so no errors will be emitted when the consumer send the "CLS" and close the tcp connection.

[mreiferson]

Ahh, good point @chaosue

[Dieterbe]

It's really confusing when nsqd logs errors that are actually not errors at all (if Conn was closed cleanly and all req were processed fine)

[mreiferson]

It's really confusing when nsqd logs errors that are actually not errors at all (if Conn was closed cleanly and all req were processed fine)

I don't think anyone disagrees with improving this, it's just about being able to deterministically identify expected EOF from unexpected.

[mreiferson]

according to the golang docs:

EOF is the error returned by Read when no more input is available. Functions should return EOF only to signal a graceful end of input. If the EOF occurs unexpectedly in a structured data stream, the appropriate error is either ErrUnexpectedEOF or some other error giving more detail.

RFR @jehiah

[mreiferson]

no errors in my testing:

[nsqd] 2015/08/08 16:01:27.656116 PROTOCOL(V2): [127.0.0.1:63472] exiting ioloop
[nsqd] 2015/08/08 16:01:27.656275 TOPIC(test): deleting channel tail433788#ephemeral
[nsqd] 2015/08/08 16:01:27.656290 CHANNEL(tail433788#ephemeral): deleting
[nsqd] 2015/08/08 16:01:27.656392 PROTOCOL(V2): [127.0.0.1:63472] exiting messagePump

[ghost]

@mreiferson Thanks.