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

Double content length #11

Open
eproxus opened this issue Feb 13, 2017 · 9 comments
Open

Double content length #11

eproxus opened this issue Feb 13, 2017 · 9 comments
Labels

Comments

@eproxus
Copy link

eproxus commented Feb 13, 2017

I get double Content-Length headers when using the master branch. E.g.:

$ http get localhost:8000/js/app.js

http: error: InvalidHeader: Content-Length contained multiple unmatching values(307288, 0)

Tracing shows this result:

GET /js/app.js HTTP/1.1
Host: localhost:8000
User-Agent: HTTPie/0.9.8
Accept-Encoding: gzip, deflate
Accept: */*
Connection: keep-alive

HTTP/1.1 206 Partial Content
Connection: Keep-Alive
Content-Length: 307288
Content-Type: application/javascript; charset=utf-8
Content-Length: 0
Content-Range: bytes 0--1/307288

...(contents of the JS-file)...

If I debug using the request_complete event as follows:

handle_event(request_complete, [_Req, _Code, _Headers, _Body, {_Timings, _Sizes}], _Args) ->
    io:format("~p ~p ~p ~p~n", [_Code, _Headers, _Body, _Sizes]),
    ok;

I get the following output:

200 [{<<"Connection">>,<<"Keep-Alive">>},
     {"Content-Length",307288},
     {"Content-Type",
      [97,112,112,108,105,99,97,116,105,111,110,47,106,97,118,97,115,99,114,
       105,112,116,59,32,99,104,97,114,115,101,116,61|<<"utf-8">>]}] <<>> [{resp_headers,
                                                                            186},
                                                                           {file,
                                                                            307288}]

Here I only see the headers set by elli_fileserve but somewhere a Content-Length: 0 header is added (and the response transformed to a 206 Partial Content as well.

This is the configuration and supervisor:

-module(myapp_sup).

-behaviour(supervisor).

% API
-export([start_link/0]).

% Supervisor callbacks
-export([init/1]).

-define(NAME, ?MODULE).

%--- API ----------------------------------------------------------------------

start_link() ->
    supervisor:start_link({local, ?NAME}, ?MODULE, []).

%--- Supervisor callbacks -----------------------------------------------------

init([]) ->
    ElliOpts = [
        {callback, elli_middleware},
        {callback_args, [{mods, [
            {elli_fileserve, [
                {path, static("js")},
                {prefix, <<"/js">>},
                {charset, <<"utf-8">>},
                {default, <<"index.html">>}
            ]}
        ]}]},
        {port, 8000}
    ],
    Elli = {
        myapp_http,
        {elli, start_link, [ElliOpts]},
        permanent,
        5000,
        worker,
        [elli]
    },
    {ok, {{one_for_all, 0, 1}, [Elli]}}.


static(Path) ->
    iolist_to_binary(filename:join([code:priv_dir(myapp), "static", Path])).

Not sure if this is a bug in elli_fileserve or Elli itself.

@yurrriq yurrriq added the bug label Feb 13, 2017
@yurrriq
Copy link
Member

yurrriq commented Feb 13, 2017

Thanks for the report. Do you notice this behaviour in develop too? I'm not sure if they're different, but in general, the develop branches of elli-lib repos have the most recent code. I'd gladly review a PR here or on elli-lib/elli if you have the time and interest.

@eproxus
Copy link
Author

eproxus commented Feb 14, 2017

Switching to the develop branch on both this middleware and on Elli itself makes no difference, unfortunately 😟

Digging a bit deeper, it turns out that Elli sets the Content-Length header itself: https://github.com/elli-lib/elli/blob/develop/src/elli_http.erl#L187

So I guess the bug is that this middleware even deals with this stuff at all. Elli seems to read all info it needs from the file again, so doing it in this middleware seems redundant.

@eproxus
Copy link
Author

eproxus commented Feb 14, 2017

One idea: since this middleware has licensing issues (#9), perhaps a rewrite is in order? I could see if creating a new elli_static from scratch would be a lot of work and if not, we could add it to the elli-lib organisation?

@yurrriq
Copy link
Member

yurrriq commented Feb 14, 2017

👍 to that. I'll set up a skeleton repo and add you. It'll mostly likely be tomorrow. If I forget, feel free to ping me.

@yurrriq
Copy link
Member

yurrriq commented Feb 14, 2017

I'd like to polish and incorporate elli_cache in elli_static as well.

@eproxus
Copy link
Author

eproxus commented Feb 15, 2017

@yurrriq Incorporating cache sounds good. So we should depend on that middleware as well? Because I can see that the elli_cache middleware is useful on its own (if you talk to a database instead of files for example, or if you have large binaries in memory with known hashes).

@eproxus
Copy link
Author

eproxus commented Feb 15, 2017

I have a simple version of elli_static working, but I'm debating the API at the moment. Not a big fan of the path, prefix and {regex, ...} redundancy. I'm wondering if it can be solved in a simpler way (somewhat similar to how the Cowboy static file serving works)... once you have created the repo we could start the discussion there and I'll add some examples.

@yurrriq
Copy link
Member

yurrriq commented Feb 15, 2017

Right, the idea behind elli_cache is to be generic.

@yurrriq
Copy link
Member

yurrriq commented Feb 16, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants