Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Client improvements #319

Closed
wants to merge 11 commits into from

3 participants

@loe

I'm opening this PR mostly to spur discussion, I don't think this is quality code yet.

I have a relatively poorly behaved server (Rails w/ Rack 1.2.5) that does not send back a Content-Length header or a Transfer-Encoding header. Nginx seems to handle this pretty well when proxying HTTP/1.0 (we are running an older version that does not proxy HTTP/1.1) by reading the whole body. I've attempted to emulate this behavior, but it sucks because I just catch all errors and decide that we're done. The HTTP/1.0 spec isn't really clear on the behavior here, its a grey area.

I think the "long-term" answer is to have a client that can handle HTTP/1.1's chunked encoding responses, which cowboy seems to have the tooling for already, it just needs to be available in the client.

Thanks! I'll continue to use this fork while I play around with my project (https://github.com/loe/boa) and hopefully cowboy:client will come-of-age in the next public release of cowboy.

@essen
Owner

There's definitely improvements to be made in the current cowboy_client. :) Will take a look.

@essen essen commented on the diff
src/cowboy_client.erl
((6 lines not shown))
{<<"host">>, FullHost},
{<<"user-agent">>, <<"Cow">>}
- |Headers],
+ ],
+ Headers2 = cowboy_req:merge_headers(Headers, DefaultHeaders),
@essen Owner
essen added a note

Yeah we do want that.

@loe
loe added a note

I'm using cowboy as a smart layer 7 proxy to rate-limit and manage incoming requests, I want to pass these through unmolested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@essen essen commented on the diff
src/cowboy_client.erl
@@ -40,7 +40,7 @@
timeout = 5000 :: timeout(), %% @todo Configurable.
buffer = <<>> :: binary(),
connection = keepalive :: keepalive | close,
- version = {1, 1} :: cowboy_http:version(),
+ version = {1, 0} :: cowboy_http:version(),
@essen Owner
essen added a note

Can't do chunked and not be 1.1 :)

@loe
loe added a note

Right, but I think to be 1.1 you need to handle chunked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@essen essen commented on the diff
src/cowboy_client.erl
@@ -212,7 +213,6 @@ stream_header(Client=#client{state=State, buffer=Buffer,
[<<>>, Rest] ->
%% If we have a body, set response_body.
Client2 = case RespBody of
- undefined -> Client#client{state=request};
@essen Owner
essen added a note

RespBody can be undefined though.

@essen Owner
essen added a note

I mean, there can be no body at all, you can't reuse it for what you do.

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

Oh I misunderstood what you were doing. Let me try again.

@essen
Owner

For response bodies, in HTTP/1.0, (the client OR the server is HTTP/1.0), the server can send the body until the connection closes. Is that what you are trying to implement?

@loe

Yes, I want it to just read until the connection is closed. I've hacked up this fork to force HTTP 1.0 and then I also need this. If cowboy:client is HTTP 1.1 my backend responds with Transfer-Encoding: chunked which also isn't implemented.

@dvv

I came to the same -- to hack to force it be 1.0. However, Google OAuth2 seems to just punt on that and serve no Content-Length:

@essen
Owner

cowboy_client will be dropped in favor of https://github.com/extend/gun which has a completely different design. I'll keep this open and make sure your ideas get in it at some point so nothing is lost.

@essen
Owner

Hello, now that cowboy_client is gone and replaced by Gun I am closing this. I am not sure Gun does everything you wanted, but it has a couple functions that allow you to override default behavior (including HTTP version) and handle edge cases so hopefully it'll be enough. If not, please open a ticket there! Thanks.

@essen essen closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 18 additions and 10 deletions.
  1. +17 −10 src/cowboy_client.erl
  2. +1 −0  src/cowboy_req.erl
View
27 src/cowboy_client.erl
@@ -40,7 +40,7 @@
timeout = 5000 :: timeout(), %% @todo Configurable.
buffer = <<>> :: binary(),
connection = keepalive :: keepalive | close,
- version = {1, 1} :: cowboy_http:version(),
+ version = {1, 0} :: cowboy_http:version(),
@essen Owner
essen added a note

Can't do chunked and not be 1.1 :)

@loe
loe added a note

Right, but I think to be 1.1 you need to handle chunked.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
response_body = undefined :: undefined | non_neg_integer()
}).
@@ -93,10 +93,11 @@ request(Method, URL, Headers, Body, Client=#client{
end,
VersionBin = cowboy_http:version_to_binary(Version),
%% @todo do keepalive too, allow override...
- Headers2 = [
+ DefaultHeaders = [
{<<"host">>, FullHost},
{<<"user-agent">>, <<"Cow">>}
- |Headers],
+ ],
+ Headers2 = cowboy_req:merge_headers(Headers, DefaultHeaders),
@essen Owner
essen added a note

Yeah we do want that.

@loe
loe added a note

I'm using cowboy as a smart layer 7 proxy to rate-limit and manage incoming requests, I want to pass these through unmolested.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Headers3 = case iolist_size(Body) of
0 -> Headers2;
Length -> [{<<"content-length">>, integer_to_list(Length)}|Headers2]
@@ -212,7 +213,6 @@ stream_header(Client=#client{state=State, buffer=Buffer,
[<<>>, Rest] ->
%% If we have a body, set response_body.
Client2 = case RespBody of
- undefined -> Client#client{state=request};
@essen Owner
essen added a note

RespBody can be undefined though.

@essen Owner
essen added a note

I mean, there can be no body at all, you can't reuse it for what you do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
0 -> Client#client{state=request};
_ -> Client#client{state=response_body}
end,
@@ -241,10 +241,17 @@ stream_header(Client=#client{state=State, buffer=Buffer,
end.
stream_body(Client=#client{state=response_body, response_body=RespBody})
- when RespBody =:= undefined; RespBody =:= 0 ->
- {done, Client#client{state=request, response_body=undefined}};
-stream_body(Client=#client{state=response_body, buffer=Buffer,
- response_body=Length}) when is_integer(Length) ->
+ when RespBody =:= done; RespBody =:= 0 ->
+ {done, Client#client{state=request, response_body=done}};
+stream_body(Client=#client{state=response_body, buffer=Buffer, response_body=undefined}) ->
+ case recv(Client) of
+ {ok, Data} ->
+ Buffer2 = << Buffer/binary, Data/binary >>,
+ stream_body(Client#client{buffer=Buffer2});
+ {error, _Reason} ->
+ {ok, Buffer, Client#client{response_body=done}}
+ end;
+stream_body(Client=#client{state=response_body, buffer=Buffer, response_body=Length}) when is_integer(Length) ->
case byte_size(Buffer) of
0 ->
case recv(Client) of
@@ -254,7 +261,7 @@ stream_body(Client=#client{state=response_body, buffer=Buffer,
{ok, Data} ->
<< Body:Length/binary, Rest/binary >> = Data,
{ok, Body, Client#client{buffer=Rest,
- response_body=undefined}};
+ response_body=done}};
{error, Reason} ->
{error, Reason}
end;
@@ -263,7 +270,7 @@ stream_body(Client=#client{state=response_body, buffer=Buffer,
{ok, Buffer, Client#client{buffer= <<>>, response_body=Length2}};
_ ->
<< Body:Length/binary, Rest/binary >> = Buffer,
- {ok, Body, Client#client{buffer=Rest, response_body=undefined}}
+ {ok, Body, Client#client{buffer=Rest, response_body=done}}
end.
recv(#client{socket=Socket, transport=Transport, timeout=Timeout}) ->
View
1  src/cowboy_req.erl
@@ -113,6 +113,7 @@
-export([lock/1]).
-export([to_list/1]).
-export([transport/1]).
+-export([merge_headers/2]).
-ifdef(TEST).
-include_lib("eunit/include/eunit.hrl").
Something went wrong with that request. Please try again.