RequestServer.Serve bugs found looking at PR-361

[puellanivis]

So, I end up moving the “tautological statement” into a function under Request (transferError(err error)). The new function better encapsulates behavior of the Request object to calls on the Request object. So, while the check now still exists, it at least is an appropriate guard statement, guarding against potential misbehavior.

The thing I noticed during PR-361, is that the break in the switch { default: … } does not break out of the for { … } loop, but rather only out of the switch. Meaning, the break as it originally appeared was actually a no-op, and execution wasn’t properly ending in that situation.

The solution was to move the for { … } loop into its own independent function, where the loop can then be broken clearly and unambiguously with a return.

Additionally, I noticed that although we’re looping over openRequests, which is supposed to only be accessed under lock, we were not actually holding the lock at that point.

As well, Request.Close really should be closing both the readerAt and the writerAt if they implement, io.Closer, even if the first one returns an err != nil error.

[eikenb]

This all looks good. I'll mark this for 1.12.0?

Anything else you wanted to add to this before merging?

[puellanivis]

No, I don’t think so. There’s a bit of a hope to avoid touching as much as possible, so as little can break as possible. 😆

[nightlyone]

Won't this wait forever now?

The previous sequence seems more correct, since you usually close the channels so workers can detect end of record stream and exit. The you wait for the exit.

[puellanivis]

The pktChan is closed as a defer in the rs.serveLoop(pktChan).

Specifically here: https://github.com/pkg/sftp/pull/363/files#diff-412115c53c6c5fea203e8253f32d2645R110

[nightlyone]

Ah yes, I overlooked somehow that the function has been factored out. So the changes you did are completely correct in this regard.

[drakkan]

I just did a quick test and it seems fine for me, all my test cases passed and also manual checking for transfer errors seems ok, thanks!

[puellanivis]

:grumble-grumble: merge conflicts when a delete and a delete/insert end up head-to-tail next to each other.

[nightlyone]

@eikenb this seems now good to go. Wanna merge it?

[puellanivis]

I’m usually hesitant to commit my own merges if there are no other checks on code. But sure, I think everyone is pretty satisfied that this won’t shouldn’t massively break anything.