Currently there's a possible race condition when shutting down the
server, which causes new connection to be added after Shutdown()

It seems like the correct behavior would be to handle the connection since it has already been accepted. In this change, the check is being done before, but the same race is still possible since there is no guarantee in execution order between checking s.done and calling s.newConn. Where is the unexpected new conn causing issues, if we can address it there.

For the ECONNRESET case, it makes sense to treat it like EOF. The other part of the change where it always returns from that branch I don't think is correct though, that could probably be broken out and explored separately.

It seems like the correct behavior would be to handle the connection since it has already been accepted. In this change, the check is being done before, but the same race is still possible since there is no guarantee in execution order between checking s.done and calling s.newConn. Where is the unexpected new conn causing issues, if we can address it there.

@dmcgowan You are right, the race condition still exists. I found the possible actual issue is addConnection may be called after closeIdleConns done and causes s.connections to be not empty. It looks a little bit hard to address this condition. I've removed the commit from this PR and will work on it later.

Thanks for the fix to this leak that we've been seeing for a while

It still seems wrong to me that the default case is to set recvErr = nil (which keeps this select case from ever unblocking again) and continuing the loop after logging the error.

Yes this fixes ECONNRESET but what happens the next time you see an esoteric error? You will have the same problem.

Why dont you just 'return' all the time, and only use the if to decide whether or not to log the error?

If your concern is not draining the other channels, then the goroutine that is writing to those channels should close them in a deferral like it does here:
https://github.com/darfux/ttrpc/blob/a03aa0459192f20913133e3d1e71419414431c6f/server.go#L351

This would guarantee that if the writer (This goroutine) dies, the channels are closed and the select isn't stuck hanging.

Why remove the nil check though? If this message is needed, then perhaps we need to change sendStatus to return error to send on recvError.

It seems to me though that this is trying to catch the shutdown case which this block is purposefully avoiding when there is active connections. If the return after this line is necessary, then the shutdown logic in this goroutine still doesn't seem correct.

Hi @benbuzbee, as discussed before, we considered other cases should be handled by the shutdown logic. But I'm agree with you, all errors should return in the block as the related connection will no longer receive requests. The shutdown case is only used for normal connection without any error. cc @dmcgowan