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.

Sign up for GitHub

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

Jump to bottom

Support for streamed response from chunked_reply when writing to non-compliant clients #548

Closed
wants to merge 6 commits into from
Closed

Support for streamed response from chunked_reply when writing to non-compliant clients #548

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0a0549b
Added in support for forcing streamed (HTTP/1.0) behaviour in chunked…
adrianroe Jun 26, 2013
e7e24d2
Merge branch 'master' of https://github.com/extend/cowboy
adrianroe Jul 4, 2013
d0e6e38
Merge branch 'master' of https://github.com/extend/cowboy
adrianroe Jul 12, 2013
8811750
Tweaks following feedback from Loic
adrianroe Jul 12, 2013
ec4fc56
Merge branch 'master' of https://github.com/extend/cowboy
adrianroe Jul 12, 2013
9efafb0
Removed emacs hints
adrianroe Jul 13, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 4 additions & 1 deletion manual/cowboy_req.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -443,7 +443,10 @@ Response related exports
>
> If the request uses HTTP/1.0, the data is sent directly
> without wrapping it in an HTTP/1.1 chunk, providing
> compatibility with older clients.
> compatibility with older clients. You can also force
> HTTP/1.0-like behaviour by calling
> `cowboy_req:set([{resp_state, waiting_streaming}], Req)`,
> to allow compatability with some non-compliant clients.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No we don't want it documented. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reasonably sure :) waiting_stream is set in the handler using cowboy_req:set- see test/http_SUITE_data/http_streamed.erl for an example (copied below) - it's called in the streamed_response test in the http_SUITE

%% Feel free to use, reuse and abuse the code in this file.

-module(http_streamed).
-behaviour(cowboy_http_handler).
-export([init/3, handle/2, terminate/3]).

init({_Transport, http}, Req, _Opts) ->
{ok, Req, undefined}.

handle(Req, State) ->
Req2 = cowboy_req:set([{resp_state, waiting_stream}], Req),
{ok, Req3} = cowboy_req:chunked_reply(200, Req2),
timer:sleep(100),
cowboy_req:chunk("streamed_handler\r\n", Req3),
timer:sleep(100),
cowboy_req:chunk("works fine!", Req3),
{ok, Req3, State}.

terminate(_, _, _) ->
ok.

Dr Adrian Roe
Director

On Saturday, 29 June 2013 at 18:45, Loïc Hoguin wrote:

In manual/cowboy_req.md (http://cowboy_req.md):

@@ -443,7 +443,10 @@ Response related exports > > > > If the request uses HTTP/1.0, the data is sent directly > > without wrapping it in an HTTP/1.1 chunk, providing > -> compatibility with older clients. > +> compatibility with older clients. You can also force > +> HTTP/1.0-like behaviour by calling > +> cowboy_req:set([{resp_state, waiting_streaming}], Req), > +> to allow compatability with some non-compliant clients.
No we don't want it documented. :)

—
Reply to this email directly or view it on GitHub (https://github.com/extend/cowboy/pull/548/files#r4952576).


### chunked_reply(StatusCode, Req) -> chunked_reply(StatusCode, [], Req)
### chunked_reply(StatusCode, Headers, Req) -> {ok, Req2}
Expand Down
47 changes: 31 additions & 16 deletions src/cowboy_req.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -951,7 +951,10 @@ reply(Status, Headers, Body, Req=#http_req{
socket=Socket, transport=Transport,
version=Version, connection=Connection,
method=Method, resp_compress=Compress,
resp_state=waiting, resp_headers=RespHeaders}) ->
resp_state=RespState, resp_headers=RespHeaders})
when RespState =:= waiting;
RespState =:= waiting_streaming ->

HTTP11Headers = if
Transport =/= cowboy_spdy, Version =:= 'HTTP/1.1' ->
[{<<"connection">>, atom_to_connection(Connection)}];
Expand Down Expand Up @@ -1090,8 +1093,10 @@ chunk(_Data, #http_req{method= <<"HEAD">>}) ->
chunk(Data, #http_req{socket=Socket, transport=cowboy_spdy,
resp_state=chunks}) ->
cowboy_spdy:stream_data(Socket, Data);
chunk(Data, #http_req{socket=Socket, transport=Transport,
resp_state=chunks, version='HTTP/1.0'}) ->
chunk(Data, #http_req{socket=Socket, transport=Transport,
version=Version, resp_state=RespState})
when Version=:='HTTP/1.0';
RespState=:=streamed ->
Transport:send(Socket, Data);
chunk(Data, #http_req{socket=Socket, transport=Transport,
resp_state=chunks}) ->
Expand Down Expand Up @@ -1128,17 +1133,19 @@ ensure_response(#http_req{resp_state=done}, _) ->
ok;
%% No response has been sent but everything apparently went fine.
%% Reply with the status code found in the second argument.
ensure_response(Req=#http_req{resp_state=waiting}, Status) ->
ensure_response(Req=#http_req{resp_state=RespState}, Status)
when RespState =:= waiting;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why you changed this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made this change because once the method is HEAD it doesn't matter what the resp_state is - we just need to return ok. Alternately we could add multiple cases (resp_state=chunks, resp_state=streamed…) but that is more brittle and (for me at least) shows the intent less well.

I've changed (removed!) the documentation and am fighting with tab etc (very different defaults on our editors!) Should have a new push in the next hour or so

Dr Adrian Roe
Director

On Saturday, 29 June 2013 at 18:46, Loïc Hoguin wrote:

In src/cowboy_req.erl:

_ = reply(Status, [], [], Req), > ok; > %% Terminate the chunked body for HTTP/1.1 only. > -ensure_response(#http_req{method= <<"HEAD">>, resp_state=chunks}, _) -> > - ok; > -ensure_response(#http_req{version='HTTP/1.0', resp_state=chunks}, _) ->
I don't understand why you changed this.

—
Reply to this email directly or view it on GitHub (https://github.com/extend/cowboy/pull/548/files#r4952582).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No I mean HTTP/1.0. You make it return the last chunk, except HTTP/1.0 doesn't do chunks so it must not be sent.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because you can only get a resp_state of chunks if you are HTTP1.1 - if it's HTTP1.0 I set the resp_state to streamed

By the way, by git skills are limited and I forgot to branch my fork when making the changes so suspect I'll need to re-merge from a fresh pull :( Apologies if you end up with another pull request!

Thanks

Adrian

Dr Adrian Roe
Director

On Friday, 12 July 2013 at 21:02, Loïc Hoguin wrote:

In src/cowboy_req.erl:

_ = reply(Status, [], [], Req), > ok; > %% Terminate the chunked body for HTTP/1.1 only. > -ensure_response(#http_req{method= <<"HEAD">>, resp_state=chunks}, _) -> > - ok; > -ensure_response(#http_req{version='HTTP/1.0', resp_state=chunks}, _) ->
No I mean HTTP/1.0. You make it return the last chunk, except HTTP/1.0 doesn't do chunks so it must not be sent.

—
Reply to this email directly or view it on GitHub (https://github.com/extend/cowboy/pull/548/files#r5173672).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I'll have to look again then to make sure I'm not missing anything. Don't worry about the branch yet.

RespState =:= waiting_streaming ->
_ = reply(Status, [], [], Req),
ok;
%% Terminate the chunked body for HTTP/1.1 only.
ensure_response(#http_req{method= <<"HEAD">>, resp_state=chunks}, _) ->
ok;
ensure_response(#http_req{version='HTTP/1.0', resp_state=chunks}, _) ->
ensure_response(#http_req{method= <<"HEAD">>}, _) ->
ok;
ensure_response(Req=#http_req{resp_state=chunks}, _) ->
_ = last_chunk(Req),
ok.
ok;
ensure_response(#http_req{}, _) ->
ok.

%% Private setter/getter API.

Expand Down Expand Up @@ -1261,19 +1268,25 @@ chunked_response(Status, Headers, Req=#http_req{
resp_headers=[], resp_body= <<>>}};
chunked_response(Status, Headers, Req=#http_req{
version=Version, connection=Connection,
resp_state=waiting, resp_headers=RespHeaders}) ->
resp_state=RespState, resp_headers=RespHeaders})
when RespState =:= waiting;
RespState =:= waiting_streaming ->
RespConn = response_connection(Headers, Connection),
HTTP11Headers = case Version of
'HTTP/1.1' -> [
{<<"connection">>, atom_to_connection(Connection)},
{<<"transfer-encoding">>, <<"chunked">>}];
_ -> []
{HTTP11Headers, NewRespState} = case {Version, RespState} of
{'HTTP/1.1', waiting} -> {[
{<<"connection">>, atom_to_connection(Connection)},
{<<"transfer-encoding">>, <<"chunked">>}],
chunks};
{'HTTP/1.1', waiting_streaming} -> {[
{<<"connection">>, atom_to_connection(Connection)}],
streamed};
_ -> {[], streamed}
end,
{RespType, Req2} = response(Status, Headers, RespHeaders, [
{<<"date">>, cowboy_clock:rfc1123()},
{<<"server">>, <<"Cowboy">>}
|HTTP11Headers], <<>>, Req),
{RespType, Req2#http_req{connection=RespConn, resp_state=chunks,
{RespType, Req2#http_req{connection=RespConn, resp_state=NewRespState,
resp_headers=[], resp_body= <<>>}}.

-spec response(cowboy:http_status(), cowboy:http_headers(),
Expand All @@ -1296,6 +1309,7 @@ response(Status, Headers, RespHeaders, DefaultHeaders, Body, Req=#http_req{
Req#http_req{resp_headers=[], resp_body= <<>>,
onresponse=already_called})
end,

ReplyType = case Req2#http_req.resp_state of
waiting when Transport =:= cowboy_spdy, Body =:= stream ->
cowboy_spdy:stream_reply(Socket, status(Status), FullHeaders),
Expand All @@ -1305,7 +1319,8 @@ response(Status, Headers, RespHeaders, DefaultHeaders, Body, Req=#http_req{
cowboy_spdy:reply(Socket, status(Status), FullHeaders, Body),
ReqPid ! {?MODULE, resp_sent},
normal;
waiting ->
WaitingState when WaitingState =:= waiting;
WaitingState =:= waiting_streaming ->
HTTPVer = atom_to_binary(Version, latin1),
StatusLine = << HTTPVer/binary, " ",
(status(Status))/binary, "\r\n" >>,
Expand Down
15 changes: 15 additions & 0 deletions test/http_SUITE.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
-export([check_raw_status/1]).
-export([check_status/1]).
-export([chunked_response/1]).
-export([streamed_response/1]).
-export([echo_body/1]).
-export([echo_body_max_length/1]).
-export([echo_body_qs/1]).
Expand Down Expand Up @@ -109,6 +110,7 @@ groups() ->
check_raw_status,
check_status,
chunked_response,
streamed_response,
echo_body,
echo_body_max_length,
echo_body_qs,
Expand Down Expand Up @@ -321,6 +323,7 @@ init_dispatch(Config) ->
cowboy_router:compile([
{"localhost", [
{"/chunked_response", http_chunked, []},
{"/streamed_response", http_streamed, []},
{"/init_shutdown", http_init_shutdown, []},
{"/long_polling", http_long_polling, []},
{"/headers/dupe", http_handler,
Expand Down Expand Up @@ -517,6 +520,18 @@ chunked_response(Config) ->
= Transport:recv(Socket, 44, 1000),
{error, closed} = cowboy_client:response(Client3).

streamed_response(Config) ->
Client = ?config(client, Config),
{ok, Client2} = cowboy_client:request(<<"GET">>,
build_url("/streamed_response", Config), Client),
{ok, 200, Headers, Client3} = cowboy_client:response(Client2),
false = lists:keymember(<<"transfer-encoding">>, 1, Headers),
{ok, Transport, Socket} = cowboy_client:transport(Client3),
{ok, <<"streamed_handler\r\nworks fine!">>}
= Transport:recv(Socket, 29, 1000),
{error, closed} = cowboy_client:response(Client3).


%% Check if sending requests whose size is around the MTU breaks something.
echo_body(Config) ->
Client = ?config(client, Config),
Expand Down
20 changes: 20 additions & 0 deletions test/http_SUITE_data/http_streamed.erl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
%% Feel free to use, reuse and abuse the code in this file.

-module(http_streamed).
-behaviour(cowboy_http_handler).
-export([init/3, handle/2, terminate/3]).

init({_Transport, http}, Req, _Opts) ->
{ok, Req, undefined}.

handle(Req, State) ->
Req2 = cowboy_req:set([{resp_state, waiting_streaming}], Req),
{ok, Req3} = cowboy_req:chunked_reply(200, Req2),
timer:sleep(100),
cowboy_req:chunk("streamed_handler\r\n", Req3),
timer:sleep(100),
cowboy_req:chunk("works fine!", Req3),
{ok, Req3, State}.

terminate(_, _, _) ->
ok.