Cannot close response when it wasn't read until EOF

[TheDSCPL]

Describe the bug
Calling response.Close() never returns.

To Reproduce
Make a Retr request and close the response without reading everything until EOF (i.e.: read just a few bytes but not the whole file).

Expected behavior
response.Close() returns without an error.

Additional context
I was able to """fix""" this by putting _, _ = io.Copy(ioutil.Discard, retrResponse) before the close call. This way the rest of the file was read and discarded so then it could be closed.

From what I can gather, this library doesn't handle closing the response if it wasn't read all the way through until the end. Reading the whole file before closing is just a waste of time and resources so my workaround should only be used as a hack to get this to work and probably not in production.

[jlaffaye]

Maybe we should have a function implementing ABORT (ABOR) for this case.

[TheDSCPL]

That would be perfect!

[TheDSCPL]

After some testing, I found out where the problem is. It's on the textproto.Conn.ReadResponse call inside Response.Close. After the connection is closed, the textproto.Conn.ReadResponse call hangs because it stays waiting for a response code that is never going to come. I don't know if this is a problem in the implementation of the server I'm connecting to but still, this should probably be a test case in the library, no? I mean, even if the connection isn't closed but just hung (unreliable network connection, for example), ReadResponse could misbehave.

[TheDSCPL]

The biggest problem for me is it just hangs. It doesn't return an error. It's just a hang. I'm trying to see if I can come up with a testable solution to make a PR with a fix!

[TheDSCPL]

Strangely enough, textproto's documentation hints that it should handle hung connections.

// A ProtocolError describes a protocol violation such
// as an invalid response or a hung-up connection.
type ProtocolError string

[TheDSCPL]

Darn... After analysing even further, I see implementing ABOR isn't going to be trivial because ABOR needs to be sent before RETR ends.

The timeline of aborting an incomplete RETR request is:

1. RETR request
2. RETR responds with 125 or 150 (without ending the response)
3. ABORT request
4. RETR ends its response with 426
5. ABORT responds with 226

The timeline of aborting a complete RETR request is (even though this case shouldn't occur because we could add a check to make sure ABOR isn't sent to a finished Response):

1. RETR request
2. RETR starts response with 125 or 150 and then ends it with 250
3. ABORT request
4. ABORT responds with 225 or 226

Correct me if I'm wrong but this seems to imply a few things:

• The id of the textproto resquest should stop being ignored (_) and instead saved in the Response struct so it can later be used to assure we're reading the right response (because ABOR will be requested before RETR has ended which means there will now be an actual use for the Pipeline inside the textproto connection object)
• The Abort method in Response instead of ServerConn so we know which response id we need to wait for before we can actually read the ABOR response

So a small rework would probably be needed in order to, for example, remove some of the ReadResponse calls that exist all over the code for a more consistent and RFC-compliant way of handling responses (using the built-in methods from the textproto standard library, i.e. StartResponse).

(sources: A, B, C)

P.S.: I'm sorry for the long text

[tgulacsi]

Just read into io.Discard in Close?