Skip to content
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

server: Flush headers when using wait=true and stream=true #877

Merged
merged 1 commit into from
Jul 8, 2014
Merged

server: Flush headers when using wait=true and stream=true #877

merged 1 commit into from
Jul 8, 2014

Conversation

cvik
Copy link

@cvik cvik commented Jul 3, 2014

Many http clients will missbehave unless they get an initial http-
response, even when long-polling. It also saves the user/client from
having to handle headers on the first action of the watch, but rather
handle the response immediately.

Many http clients will missbehave unless they get an initial http-
response, even when long-polling. It also saves the user/client from
having to handle headers on the first action of the watch, but rather
handle the response immediately.
@philips
Copy link
Contributor

philips commented Jul 7, 2014

lgtm, do you have an example client that I can add to the changelog?

@bmizerany
Copy link
Contributor

lgtm

@cvik
Copy link
Author

cvik commented Jul 7, 2014

Hi,

actually i suppose most clients can handle it, as long as you give them a
sufficient long timeout before they expect a response.

The clients i was working with was the standard client of erlang (httpc)
and a popular replacement called lhttpc. After i sent this pull request i
made some more digging and discovered that they can handle it if, as i
mentioned, you give them a long (or infinite) timeout.

Don't know if the patch is really necessary, it might make asynchronous
programming easier with some clients i suppose. Your call.

2014-07-07 23:31 GMT+02:00 Blake Mizerany notifications@github.com:

lgtm


Reply to this email directly or view it on GitHub
#877 (comment).

@yichengq
Copy link
Contributor

yichengq commented Jul 7, 2014

lgtm also.

@philips
Copy link
Contributor

philips commented Jul 7, 2014

@rawn If headers are given they have a timeout for that already?

@cvik
Copy link
Author

cvik commented Jul 7, 2014

The library i'm using currently is process oriented (most of erlang is)
which means that if you make a request with the http library it will spin
up an erlang process and return a Pid (erlang process id) which you use to
get the body of the request (through the process). This call will not
return until it get's the headers and has a default timeout (blocking is
something you usually avoid by default in erlang).

@cvik
Copy link
Author

cvik commented Jul 7, 2014

No idea if i answered your question btw.

2014-07-08 1:21 GMT+02:00 Christoffer Vikström chvi77@gmail.com:

The library i'm using currently is process oriented (most of erlang is)
which means that if you make a request with the http library it will spin
up an erlang process and return a Pid (erlang process id) which you use to
get the body of the request (through the process). This call will not
return until it get's the headers and has a default timeout (blocking is
something you usually avoid by default in erlang).

@philips
Copy link
Contributor

philips commented Jul 7, 2014

@rawn And this patch helped relieve this blocking? Did you test it?

@cvik
Copy link
Author

cvik commented Jul 7, 2014

Yes, it helped.
Den 8 jul 2014 01:41 skrev "Brandon Philips" notifications@github.com:

@rawn https://github.com/Rawn And this patch helped relieve this
blocking? Did you test it?


Reply to this email directly or view it on GitHub
#877 (comment).

@cvik
Copy link
Author

cvik commented Jul 7, 2014

I'm writing a new erlang client for etcd, thats how i noticed.
Den 8 jul 2014 01:45 skrev chvi77@gmail.com:

Yes, it helped.
Den 8 jul 2014 01:41 skrev "Brandon Philips" notifications@github.com:

@rawn https://github.com/Rawn And this patch helped relieve this
blocking? Did you test it?


Reply to this email directly or view it on GitHub
#877 (comment).

@philips
Copy link
Contributor

philips commented Jul 8, 2014

@rawn great, thanks. Merging.

philips added a commit that referenced this pull request Jul 8, 2014
server: Flush headers when using wait=true and stream=true
@philips philips merged commit 43791a2 into etcd-io:master Jul 8, 2014
@cvik
Copy link
Author

cvik commented Jul 8, 2014

Thanks!
Den 8 jul 2014 02:04 skrev "Brandon Philips" notifications@github.com:

Merged #877 #877.


Reply to this email directly or view it on GitHub
#877 (comment).

@cvik cvik deleted the flush-headers-on-wait-stream branch June 24, 2015 00:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants