streamed_response functionality #471

Closed
wants to merge 4 commits into
from

Conversation

Projects
None yet
2 participants
Contributor

adrianroe commented Mar 18, 2013

Analogous to chunked_response, this provides streamed_respone stream functions
to allow streaming to HTTP 1.1 clients that do not support chunking

Owner

essen commented Mar 18, 2013

Have you looked at set_resp_body_fun?

Contributor

adrianroe commented Mar 18, 2013

Yes I have - and I've implemented a solution using it too, but it was awkward to do. In particular we need to get state back to the terminate function (which we use for logging etc) and short of using the process dictionary etc to do so that's not easy to do. We basically have code that says:

reply (chunked_reply or streamed_reply - depending on the client signature)

info -> send the latest TV content (again, chunked or streamed) and update state with stats

Terminate -> log useful stuff (like #bytes sent, duration) etc

The same structure with resp_body_fun is ugly (unless I'm missing something obvious). I even put a note in the updated docs pointing people at set_resp_body_fun alongside the streamed_reply docs as for some use cases I have no doubt it would be a good way to go.

All the best

Adrian

Dr Adrian Roe
Director

On Monday, 18 March 2013 at 14:33, Loïc Hoguin wrote:

Have you looked at set_resp_body_fun?


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

Contributor

adrianroe commented Mar 19, 2013

I fixed a bug related to ensure_response (which I had in my original patch but failed to migrate in the pull).

Contributor

adrianroe commented Apr 2, 2013

Loic

Can I give a gentle nudge on this one - I'd love to see this integrated as it make our life a lot easier and I'm sure would also help other people that need to stream to non-compliant clients…

Thanks for your help,

Adrian

Dr Adrian Roe
Director

On Monday, 18 March 2013 at 14:42, Adrian Roe wrote:

Yes I have - and I've implemented a solution using it too, but it was awkward to do. In particular we need to get state back to the terminate function (which we use for logging etc) and short of using the process dictionary etc to do so that's not easy to do. We basically have code that says:

reply (chunked_reply or streamed_reply - depending on the client signature)

info -> send the latest TV content (again, chunked or streamed) and update state with stats

Terminate -> log useful stuff (like #bytes sent, duration) etc

The same structure with resp_body_fun is ugly (unless I'm missing something obvious). I even put a note in the updated docs pointing people at set_resp_body_fun alongside the streamed_reply docs as for some use cases I have no doubt it would be a good way to go.

All the best

Adrian

Dr Adrian Roe
Director

On Monday, 18 March 2013 at 14:33, Loïc Hoguin wrote:

Have you looked at set_resp_body_fun?


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

Owner

essen commented Apr 11, 2013

I'll get to it in the next few days.

Contributor

adrianroe commented Apr 11, 2013

Many thanks

Dr Adrian Roe
Director

On Thursday, 11 April 2013 at 22:22, Loïc Hoguin wrote:

I'll get to it in the next few days.


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

Owner

essen commented Apr 25, 2013

I'm thinking we should do something like we did for HTTP/1.0. Perhaps you could just set the version to 1.0 (which would disable chunked) just before you use the chunk functions. What do you think? This can be done with:

Req2 = cowboy_req:set([{version, {1, 0}}], Req)

Then use the functions as normal.

This would be in my opinion better than supporting extra API to do essentially the same thing.

Contributor

adrianroe commented Apr 25, 2013

I toyed with that approach at the start and it is certainly better from a code clutter perspective. The main challenge then is that we'll send a HTTP/1.0 response header as well which precludes keep alive and when streaming to set top boxes would mean the server would not comply with the various standards (which insist on HTTP/1.1). Not of course that the clients adhere to the spec (hence requiring the ability to not use chunked for certain clients). The set top box (TV) solution interoperates with over the air (broadcast) TV and is regulated, meaning that releases go through a standards compliance check before the servers can be added to the broadcast white-list.

I even wondered for a while whether you could force the the HTTP/1.1 response while keeping HTTP/1.0-like behaviour but didn't really get anywhere - the call to "response" (which writes the HTTP version out) is made from the chunked_reply function so you don't have an opportunity to change the version back before the header is written. You could have a nasty hack where you set e.g. {-1, 1} as the version and then have chunked response patch it back once it had decided on headers etc.

My (strong) preference is still for a separate function but I concede that my use-case is not particularly mainstream.

All the best

Adrian

Dr Adrian Roe
Director

On Thursday, 25 April 2013 at 19:51, Loïc Hoguin wrote:

I'm thinking we should do something like we did for HTTP/1.0. Perhaps you could just set the version to 1.0 (which would disable chunked) just before you use the chunk functions. What do you think? This can be done with:
Req2 = cowboy_req:set([{version, {1, 0}}], Req)

Then use the functions as normal.
This would be in my opinion better than supporting extra API to do essentially the same thing.


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

Owner

essen commented Apr 25, 2013

Makes sense. I think we are going to merge this the way it is. I had a few other ideas but none satisfactory enough. Function calls are explicit which is good.

I want to merge something else first and then we will be able to go over the details to get this in. Thanks.

Owner

essen commented May 31, 2013

I'm sorry I haven't merged this yet, I can't stop from feeling there must be a better solution to do this than adding many new functions.

Probably can define a resp_state for streaming, that you can set using cowboy_req:set/2, and when you call chunked_reply and chunk after that it correctly not use chunks while still allowing for the same mechanism.

Contributor

adrianroe commented May 31, 2013

How about something like the attached - not tested etc - more to keep the discussion going as I also share your dislike of the separate function mechanism… You'd need to call set([{resp_state, waiting_streamed}]) before calling chunked_reply() and then repeatedly chunk(). About my only concern is that set is marked as private…

All the best

Adrian

diff --git a/src/cowboy_req.erl b/src/cowboy_req.erl
index 093663c..462884d 100644
--- a/src/cowboy_req.erl
+++ b/src/cowboy_req.erl
@@ -166,7 +166,7 @@

    %% Response.
    resp_compress = false :: boolean(),
  •   resp_state = waiting :: locked | waiting | chunks | done,
    
  •   resp_state = waiting :: locked | waiting | waiting_streamed | chunks | streamed | done,
    resp_headers = [] :: cowboy:http_headers(),
    resp_body = <<>> :: iodata() | resp_body_fun()
            | {non_neg_integer(), resp_body_fun()}
    
    @@ -1077,11 +1077,11 @@ 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='HTTP/1.0'}) ->
    Transport:send(Socket, Data);
    -chunk(Data, #http_req{socket=Socket, transport=Transport,
  •           resp_state=chunks}) ->
    
    +chunk(Data, #http_req{socket=Socket, transport=Transport, resp_state=streamed}) ->
  •   Transport:send(Socket, Data);
    
    +chunk(Data, #http_req{socket=Socket, transport=Transport, resp_state=chunks}) ->
    Transport:send(Socket, [integer_to_list(iolist_size(Data), 16),
    <<"\r\n">>, Data, <<"\r\n">>]).

@@ -1119,13 +1119,14 @@ ensure_response(Req=#http_req{resp_state=waiting}, Status) ->
_ = 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.

@@ -1247,19 +1248,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(),

Dr Adrian Roe
Director

On Friday, 31 May 2013 at 13:58, Loïc Hoguin wrote:

I'm sorry I haven't merged this yet, I can't stop from feeling there must be a better solution to do this than adding many new functions.
Probably can define a resp_state for streaming, that you can set using cowboy_req:set/2, and when you call chunked_reply and chunk after that it correctly not use chunks while still allowing for the same mechanism.


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

Owner

essen commented Jun 2, 2013

It's not publicly documented yet, but it's necessary for some middlewares, so I'm thinking of documenting it with a note that the function shouldn't be used outside middlewares (of course in your case you'd be forced to use it outside). It's here to stay either way as the Req object is opaque.

Contributor

adrianroe commented Jun 2, 2013

I'll test and do a pull request then. 

​Adrian 


Sent from Mailbox for iPhone

On Sun, Jun 2, 2013 at 6:52 PM, Loïc Hoguin notifications@github.com
wrote:

It's not publicly documented yet, but it's necessary for some middlewares, so I'm thinking of documenting it with a note that the function shouldn't be used outside middlewares (of course in your case you'd be forced to use it outside). It's here to stay either way as the Req object is opaque.

Reply to this email directly or view it on GitHub:
extend#471 (comment)

Owner

essen commented Jun 29, 2013

Closing this since we have the other PR for it. Thanks!

essen closed this Jun 29, 2013

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