client: textproto's bufio design causes half closed idle connections

[kayrus]

I'm implementing SMTP connections pool in my app (see #154) and I already faced two issues when a remote SMTP server closes an idle connection due to timeout:

• impossible to check the connection state in advance before sending an email / check connection, see closed Add Noop method to the Session interface #182
• idle connection closed by a remote server end up in half closed mode: CLOSE_WAIT consuming the OS descriptor unless you run a command within the client's public methods or explicitly close the connection which doesn't make sense for a connections pool.

SMTP client is not aware about the closed connection, because textproto uses bufio.Reader under the hood and due to client's Command/Response approach bufio buffers the timeout response until the next Command/Response action.

See a telnet example:

$ openssl s_client  -crlf -quiet -connect email-smtp.eu-central-1.amazonaws.com:587 -starttls smtp 
depth=2 C = US, O = Amazon, CN = Amazon Root CA 1
verify return:1
depth=1 C = US, O = Amazon, OU = Server CA 1B, CN = Amazon
verify return:1
depth=0 CN = email-smtp.eu-central-1.amazonaws.com
verify return:1
250 Ok
EHLO test
250-email-smtp.amazonaws.com
250-8BITMIME
250-STARTTLS
250-AUTH PLAIN LOGIN
250 Ok
451 4.4.2 Timeout waiting for data from client. << is sent by a server after 30 secs

See the last 451 4.4.2 Timeout waiting for data from client. response. It is sent by remote server and not recognized by the SMTP client until you call any other command.

Not sure if this is a go-smtp problem, but an upstream textproto package issue. Nevertheless this unexpected behavior has a workaround in net/http.Transport package: https://github.com/golang/go/blob/e7c56fe9948449a3710b36c22c02d57c215d1c10/src/net/http/transport.go#L2092

I developed a couple of workarounds. One is based on substituting the c.Text.Reader.R reader and writeAhead approach, another is based on readAhead approach by implementing a custom Read method and wrapping the c.conn. Both workarounds work more or less, but the second one is more cleaner in terms of the code. The workaround detects io.EOF right after the remote server closes the connection and it is possible to close the c.Text to fully close the descriptor.

Unfortunately when a plain connection is upgraded to StartTLS, some problems popup in rare cases. Solving these problems require overengineering with channels, selectors, etc. Not sure if I'm on the right way.

Is it the first time you hear about this problem? Does it make sense to solve it?

[emersion]

I'd recommend regularly sending Noop commands and pruning connections where that fails. Or send Noop when extracting a connection from the pool.

[kayrus]

@emersion sending Noop() adds an additional latency if you call it right before returning a valid connection from a pool. Regular Noop() sending also adds code complexity, useless loops and CPU cycles. Besides you closed the Noop() PR.
At the end It'd be more cleaner to know that connection is closed in event based manner. And yes, I'd still would like to have Noop() method in Session interface independently, because smtp.Session interface is used in my app along with go-smtp-proxy repo.