fix(x): return ErrClosed for abnormal closures#377
Conversation
d5131c5 to
f19b02e
Compare
| if websocket.IsCloseError(err, websocket.CloseNormalClosure) { | ||
| return 0, io.EOF | ||
| } | ||
| return 0, fmt.Errorf("%w %w", net.ErrClosed, closeError) |
There was a problem hiding this comment.
I think this is a good solution. We are encoding that Websockets is done in case of error.
Perhaps for another PR, we should still consider fixing the calling loop, so the same issue doesn't happen with future protocols. My thought is that we should break the loop on any error, except io. ErrShortBuffer.
There was a problem hiding this comment.
I added the breaking out of the calling loop in: OutlineFoundation/tunnel-server@9621558 Is that not what you mean?
There was a problem hiding this comment.
Yes, I forgot about that. But I was also thinking about errors after the association is established.
The timeout should take care of that, but we may run into issues because Write will update the timeout regardless of the error:
https://github.com/Jigsaw-Code/outline-ss-server/blob/962155844aed338b15b5d322212e87102b00e096/service/udp.go#L456
So we may have a situation where the writes keep happening and failing, with the association broken, but kept alive.
I believe we should fix the code to only update the deadline on successful writes.
There was a problem hiding this comment.
Got it. That makes sense. I don't think that would be a regression, I'm pretty sure we had that behavior before the refactor, but I agree it would be good to be more strict there. I'll make a note to address it.
Co-authored-by: Vinicius Fortuna <fortuna@users.noreply.github.com>
* fix(x): return ErrClosed for abnormal closures * Wrap error in `net.ErrClosed` instead of replacing it. * Update x/websocket/endpoint.go Co-authored-by: Vinicius Fortuna <fortuna@users.noreply.github.com> --------- Co-authored-by: Vinicius Fortuna <fortuna@users.noreply.github.com>
Errors returned from a Gorilla connection's
NextReader()call require applications to break out of the application's read loop, as errors are permanent. Subsequent calls return the same error and after 1000 calls panic:So we return
net.ErrClosederror for all other close errors. This will ensure the UDP loop knows to exit as the connection has closed.