New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve view response errors #1

Merged
merged 4 commits into from Aug 23, 2011

Conversation

Projects
None yet
2 participants
@kocolosk
Member

kocolosk commented Aug 15, 2011

No description provided.

@kocolosk

This comment has been minimized.

Show comment
Hide comment
@kocolosk

kocolosk Aug 15, 2011

Member

The current patch set cleanly terminates the chunked response in case of a mid-response error, which is not what we want. Instead we want to skip the final empty chunk and ensure that the socket is closed on the way out. I think we can accomplish the latter by setting 'mochiweb_request_force_close' in the process dictionary, while for the former we need to skip the call to end_json_response somehow.

Member

kocolosk commented Aug 15, 2011

The current patch set cleanly terminates the chunked response in case of a mid-response error, which is not what we want. Instead we want to skip the final empty chunk and ensure that the socket is closed on the way out. I think we can accomplish the latter by setting 'mochiweb_request_force_close' in the process dictionary, while for the former we need to skip the call to end_json_response somehow.

@rnewson

This comment has been minimized.

Show comment
Hide comment
@rnewson

rnewson Aug 17, 2011

Member

Will fix; was up late.

Member

rnewson commented Aug 17, 2011

Will fix; was up late.

@kocolosk

This comment has been minimized.

Show comment
Hide comment
@kocolosk

kocolosk Aug 17, 2011

Member

New code looks quite good. I think I'd add an access log entry in the http_abort clause which logs the usual request information but indicates somehow that the response was aborted midway through (instead of a status code, perhaps)?

Member

kocolosk commented Aug 17, 2011

New code looks quite good. I think I'd add an access log entry in the http_abort clause which logs the usual request information but indicates somehow that the response was aborted midway through (instead of a status code, perhaps)?

@kocolosk

View changes

Show outdated Hide outdated src/chttpd.erl
"" -> {ok, Resp1};
_ -> send_delayed_chunk(Resp1, FirstChunk)
end,
send_delayed_chunk(Resp2, Chunk);

This comment has been minimized.

@kocolosk

kocolosk Aug 19, 2011

Member

Could this clause be written more simply?

{ok, Resp} = StartFun(Req, Code, Headers),
send_delayed_chunk(Resp, [FirstChunk, Chunk]);

@kocolosk

kocolosk Aug 19, 2011

Member

Could this clause be written more simply?

{ok, Resp} = StartFun(Req, Code, Headers),
send_delayed_chunk(Resp, [FirstChunk, Chunk]);

This comment has been minimized.

@rnewson

rnewson Aug 19, 2011

Member

yup, got this straight from Paul's original, wasn't focused on brevity there. Will fix.

@rnewson

rnewson Aug 19, 2011

Member

yup, got this straight from Paul's original, wasn't focused on brevity there. Will fix.

@kocolosk

View changes

Show outdated Hide outdated src/chttpd_show.erl
send_non_empty_chunk(Resp, Chunk),
{ok, Acc#lacc{resp=Resp}}.
{ok, Resp} = chttpd:start_delayed_chunked_response(Req, Code, JsonHeaders),
{ok, Resp1} = send_non_empty_chunk(Resp, Chunk),

This comment has been minimized.

@kocolosk

kocolosk Aug 19, 2011

Member

Does it make sense to have a start_delayed_chunked_response/4 analogous to the JSON version here and delay longer before sending that first chunk?

@kocolosk

kocolosk Aug 19, 2011

Member

Does it make sense to have a start_delayed_chunked_response/4 analogous to the JSON version here and delay longer before sending that first chunk?

This comment has been minimized.

@rnewson

rnewson Aug 22, 2011

Member

it does make sense, will change.

@rnewson

rnewson Aug 22, 2011

Member

it does make sense, will change.

Robert Newson added some commits Jul 21, 2011

Delay starting a response for errors
If we start a response too early we can't respond with errors
appropriately. This delays the start of the response until we
have heard back from the cluster that there's at least one
message to send the client.

Errors that occur before any message result in a response that
has the correct 500 status code. Errors that occur after a
response has started are handled by sending nothing and closing
the connection.

Thanks Paul Davis and Bob Dionne.

BugzID: 12438
Improve error handling for _list responses
This patch delays sending a _list response until the first row of the
view has been consumed.  Errors that occur before that event result in
a response that has the correct 500 status code. Errors that occur
after a response has started are handled by sending nothing and closing
the connection.

BugzID: 12438
@kocolosk

This comment has been minimized.

Show comment
Hide comment
@kocolosk

kocolosk Aug 22, 2011

Member

@rnewson, does the rebased patch set work for you? One for views introducing the setup, one for _list, one for tests, one for logging.

Member

kocolosk commented Aug 22, 2011

@rnewson, does the rebased patch set work for you? One for views introducing the setup, one for _list, one for tests, one for logging.

@kocolosk kocolosk merged commit 24108e0 into master Aug 23, 2011

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment