Skip to content

Loading…

Prevent Unnecessary Socket Exceptions #79

Closed
wants to merge 1 commit into from

3 participants

@jdhood1

We were seeing a lot of seemingly harmless socket exceptions in our system. After a little digging, I found out that it's because the client is closing the socket as soon as it receives a complete message from mochiweb:respond(). Unfortunately, mochiweb:respond() would then try to send the body, even when it is blank (""), which was causing the exception. This change cleans up those unnecessary exceptions.

@jdhood1 jdhood1 We were seeing a lot of seemingly harmless socket exceptions in our s…
…ystem. After a little digging, I found out that it's because the client is closing the socket as soon as it receives a complete message from mochiweb:respond(). Unfortunately, mochiweb:respond() would then try to send the body, even when it is blank (""), which was causing the exception. This change cleans up those unnecessary exceptions.
49d3779
@etrepum
Mochi Media, Inc. member

I'm not so sure that this is the correct fix, it would be helpful to see exactly what the exceptions look like and ideally there would be a test for this.

@jdhood1

Yah, I didn't like having the blank string hard-coded, especially when it's iodata(). Perhaps 'iolist_size(Body) == 0' would be a more appropriate check.

I've been trying all morning to reproduce this problem consistently in a unit test, but I haven't yet. I'll keep trying and send you the unit test, if I finally get it to reproduce the problem.

@etrepum
Mochi Media, Inc. member

I'd at least like to look at the exception, even if you can't reliably reproduce it.

@etrepum
Mochi Media, Inc. member

Also there's already a call to iolist_size earlier in the function, so you could extract that into a variable to avoid calling it twice (you probably wouldn't be able to do it in a guard anyway)

@doubleyou
Mochi Media, Inc. member

This code won't even work, since for a default value in the 'if' statement you should use 'true', not '_'.

@jdhood1

You're absolutely right. Clearly, the web interface should only be used by experts. I've submitted another pull request with the correct code change that compiles and passes all unit tests.

@jdhood1 jdhood1 closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Jun 22, 2012
  1. @jdhood1

    We were seeing a lot of seemingly harmless socket exceptions in our s…

    jdhood1 committed
    …ystem. After a little digging, I found out that it's because the client is closing the socket as soon as it receives a complete message from mochiweb:respond(). Unfortunately, mochiweb:respond() would then try to send the body, even when it is blank (""), which was causing the exception. This change cleans up those unnecessary exceptions.
Showing with 4 additions and 5 deletions.
  1. +4 −5 src/mochiweb_request.erl
View
9 src/mochiweb_request.erl
@@ -320,11 +320,10 @@ respond({Code, ResponseHeaders, chunked}) ->
start_response({Code, HResponse1});
respond({Code, ResponseHeaders, Body}) ->
Response = start_response_length({Code, ResponseHeaders, iolist_size(Body)}),
- case Method of
- 'HEAD' ->
- ok;
- _ ->
- send(Body)
+ if Method == 'HEAD'; Body == "" ->
+ ok;
+ _ ->
+ send(Body)
end,
Response.
Something went wrong with that request. Please try again.