erlyaws / yaws

Yaws webserver
https://erlyaws.github.io
BSD 3-Clause "New" or "Revised" License
1.29k stars 267 forks source link

Crashes when setting socket options #410

Closed nthauvin closed 4 years ago

nthauvin commented 4 years ago

We are migrating our yaws servers to Debian Buster (OTP 21 / yaws-2.0.6), and we are observing some new crashes in the logs:

Yaws process died: {{badmatch,{error,closed}},
                    [{yaws,setopts,3,[{file,"yaws.erl"},{line,2403}]},
                     {yaws,http_recv_request,2,
                           [{file,"yaws.erl"},{line,2425}]},
                     {yaws,do_http_get_headers,2,
                           [{file,"yaws.erl"},{line,2406}]},

However, these crashes do not seem to have an impact on http requests. Tracing these, it seems there is now a different timing on OTP side when receiving F packets. In particular, a socket opened with keep alive for a single curl request will crash in the second iteration of yaws_server:aloop/4 whereas it was not the case before (OTP-19 / yaws-2.0.2).

That seems to happen only for SSL sockets, so we catched the ssl:setopts call to get rid of these reports (see hackish setopts_crash_patch.txt

When digging the issue, we found that OTP had several commits about setopts, for example: https://github.com/erlang/otp/commit/1142306a703d5c6f0dc67efee0efe0fd7f9d8d06

Yaws is not the only one ;)

vinoski commented 4 years ago

Thanks for your report, I'll look into it.

vinoski commented 4 years ago

@nthauvin, a question: is the catch on ssl:setopts really necessary? Seems like that could mask bigger problems than just an {error, closed} mismatch.

nthauvin commented 4 years ago

That's a good question. I just did the same thing as OTP http_transport:setopts/3 without really thinking about all the consequences on the calling functions. However, in most of the cases, socket errors seem properly handled by following instructions (mainly yaws:do_recv) whereas ssl crashes brutally with {error, closed} or einval EXITs (hence the catch) I understood the usage of setopts as an attempt to normalize future flows rather than processing socket errors.

vinoski commented 4 years ago

Great, thank you for the information, especially your final sentence, which makes an excellent point.