Fix list HTTP header handling. #18

Closed
wants to merge 3 commits into
from

Projects

None yet

5 participants

@calebcase

Currently calls to getRow() cause the HTTP headers to be sent immediately back
to the client. This happens even if an error is thrown after the getRow(), but
before any send(...) or start(...). Worse, if a list throws an exception an
extra, invalid header is sent to the client (resulting in various bad
behavior).

Erlang list handling will now wait until data has been sent BEFORE sending the
HTTP headers to the client. If an error is reported it will result in an HTTP
error code as expected. This does not change the behavior of errors thrown
AFTER data has been sent: They will still result in an HTTP 200 even if an
error is reported.

The line protocol between Erlang and os processes has been extended to support
an optional Header field on "chunks" and "end". The javascript list handling
has been updated to use this if a new header is set via start(...). This makes
it possible to begin processing with getRow(), but later reset the headers via
start(...). Again, if data has been sent(...) the new headers will NOT take
effect.

COUCHDB-430
COUCHDB-514
COUCHDB-764

@calebcase calebcase Fix list HTTP header handling.
Currently calls to getRow() cause the HTTP headers to be sent immediately back
to the client. This happens even if an error is thrown after the getRow(), but
before any send(...) or start(...). Worse, if a list throws an exception an
extra, invalid header is sent to the client (resulting in various bad
behavior).

Erlang list handling will now wait until data has been sent BEFORE sending the
HTTP headers to the client. If an error is reported it will result in an HTTP
error code as expected. This does not change the behavior of errors thrown
AFTER data has been sent: They will still result in an HTTP 200 even if an
error is reported.

The line protocol between Erlang and os processes has been extended to support
an optional Header field on "chunks" and "end". The javascript list handling
has been updated to use this if a new header is set via start(...). This makes
it possible to begin processing with getRow(), but later reset the headers via
start(...). Again, if data has been sent(...) the new headers will NOT take
effect.

COUCHDB-430
COUCHDB-514
COUCHDB-764
8e8501c
@zdzolton

Awesome! I tried to fix this a long time ago, but didn't want to mess with the view server protocol on the Erlang side...

If you'd like, I provided a spec for this (it's the render.diff attachment) on one of those ancient tickets:
https://issues.apache.org/jira/browse/COUCHDB-514

@calebcase

:o} Thanks! I used that and the other patches on your bug (and the other bugs as well) as the basis for my fix. Let me know if you get a chance to test it out. I'm interested to find any bugs!

@dch
dch commented Apr 21, 2012

Nice!! Thanks for the proposed patch. One of the other devs will need to look at it tho.

It would be a huge ++ to have a test that demonstrates the failure before your patch, and the success afterwards. Do you want to check out https://issues.apache.org/jira/secure/attachment/12424604/list_views.diff and see if you can get it to apply cleanly, and then add it into your PR?

@aronwoost

+1, let's make this happen

@dch
dch commented Nov 1, 2012

Let's see if we can get this into 1.3.0 - branching on 5th November! @zdzolton @aronwoost @calebcase do you want to integrate the test code above?

@calebcase

Just now seeing your comment @dch. Running the tests now with your test case.

@calebcase

@dch see ac2240b

Before I merged with upstream only the '27 form_submit' test failed with my patch and your test applied (appears to be unrelated, was failing upstream as well).

After the merge many tests are failing with 'Reason: You are not a server admin.' It all starts to go bad after:

not ok 46 replication
Reason: expected '"joe"', got 'null'

Will have to investigate later when I have more time.

@dch
dch commented Nov 5, 2012

On 5 November 2012 12:00, Caleb Case notifications@github.com wrote:

@dch https://github.com/dch see ac2240bhttps://github.com/apache/couchdb/commit/ac2240bcebeee92acdb343f55e58435bce235194

Before I merged with upstream only the '27 form_submit' test failed with
my patch and your test applied (appears to be unrelated, was failing
upstream as well).

After the merge many tests are failing with 'Reason: You are not a server
admin.' It all starts to go bad after:

That happens sometimes in the tests, not your fault. A re-run usually goes
fine.

@calebcase

Yup, re-run and the tests all passed. Looks good from my perspective, is there anything else upstream needs to consider merging?

@dch
dch commented Nov 6, 2012

Just need to get some eyes on a reviewer - onto it & many thanks Caleb! I've put the info into https://issues.apache.org/jira/browse/COUCHDB-430 from here on.

@aronwoost feel free to take a look & @zdzolton, thanks for your work on the earlier iterations!

On 5 November 2012 20:58, Caleb Case notifications@github.com wrote:

Yup, re-run and the tests all passed. Looks good from my perspective, is
there anything else upstream needs to consider merging?


Reply to this email directly or view it on GitHubhttps://github.com/apache/couchdb/pull/18#issuecomment-10085261.

@janl
Member
janl commented Nov 14, 2012

We committed this to master, thanks for all your work!

Could you close the PR for us? (we can’t at the moment :)

@aronwoost

Yeah!

@calebcase

Woot!

@calebcase calebcase closed this Nov 15, 2012
@zdzolton

Awesome work guys! Really increases the number of ways you can use couch
without wrapping a web tier around it...

On Thu, Nov 15, 2012 at 3:01 PM, Caleb Case notifications@github.comwrote:

Woot!


Reply to this email directly or view it on GitHubhttps://github.com/apache/couchdb/pull/18#issuecomment-10425219.

@rnewson rnewson pushed a commit to rnewson/couchdb that referenced this pull request Mar 10, 2013
@kocolosk kocolosk Merge branch '12220-nodedown-handling-db', close #18
Conflicts:
	src/fabric_util.erl

BugzID: 12220
17644b6
@hdiedrich hdiedrich pushed a commit to hdiedrich/couchdb that referenced this pull request May 19, 2015
@mikewallace1979 mikewallace1979 Convert {timeout, Error} errors to timeout
If chttpd got a {timeout, Error} error then it would return
that tuple to the client. Since Error is some internal specifics
this isn't particularly useful information to return to the client.

This commit converts it to our usual timeout response.

This closes #18

COUCHDB-2425

Signed-off-by: Alexander Shorin <kxepal@apache.org>
1ca78bf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment