ninenines / gun

HTTP/1.1, HTTP/2, Websocket client (and more) for Erlang/OTP.
ISC License
891 stars 232 forks source link

Disconnect when setopts returns an error #250

Closed zuiderkwast closed 3 years ago

zuiderkwast commented 3 years ago

There is a race condition: After receiving data, before setting the socket back to {active, once}, the server may have closed the socket or an error may have occurred. Since the socket was in passive mode, no message is received. Handling the return value of setopts solves the issue.


This was discovered when investigating failing and unstable test cases in Cowboy. The cause was traced back to Gun (which is used for testing Cowboy). The Cowboy test case waits for gun:await_body to return {error, {stream_error, closed}} which never comes. Cowboy test cases which (sometimes or always) got stuck until timeout or time trap in this way, and solved by this fix, are these:

essen commented 3 years ago

OK that can't possibly work because you are simply calling disconnect/2 and this function returns a state change tuple, not state data. I will close this PR and do a better fix because I need to make Cowboy green in order to release it today. Huge thanks for figuring out where the problem comes from though!

zuiderkwast commented 3 years ago

OK that can't possibly work because you are simply calling disconnect/2 and this function returns a state change tuple, not state data.

Oh, I didn't check that. :) I assumed it would exit and not return at all.

Simplest solution (one that you probably don't like) is to fake a message from the socket by doing self() ! {ssl_closed, Socket} from within active/1 and let gen_statem handle it.

essen commented 3 years ago

No because you'll queue the message after all that are already waiting. I'm just handling the return value, it's only called 10 times.