elli-lib / elli_fileserve

Development has moved to elli_static.
https://github.com/elli-lib/elli_static
0 stars 4 forks source link

Double content length #11

Open eproxus opened 7 years ago

eproxus commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

:+1: 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 commented 7 years ago

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

eproxus commented 7 years ago

@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 commented 7 years ago

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 commented 7 years ago

Right, the idea behind elli_cache is to be generic.

yurrriq commented 7 years ago

https://github.com/elli-lib/elli_static