ninenines / cowlib

Support library for manipulating Web protocols.
ISC License
279 stars 173 forks source link

Got error when sending trailers #92

Closed tony612 closed 4 years ago

tony612 commented 4 years ago

The syntax is Elixir, but similar with Erlang. cowlib version: 2.8.0

19:30:42.279 [error] Ranch listener "Interop.Endpoint" had connection process started with :cowboy_clear:start_link/4 at #PID<0.443.0> exit with reason:
{
  {:case_clause,
    {:trailers,
      [{"grpc-message", ""}, {"grpc-status", "0"}]
    }
  },
  [
    {:cow_http2_machine, :queue_data, 4, [file: '/Users/tony/repo/tony612/grpc-elixir/interop/deps/cowlib/src/cow_http2_machine.erl', line: 1339]},
    {:cow_http2_machine, :send_or_queue_data, 4, [file: '/Users/tony/repo/tony612/grpc-elixir/interop/deps/cowlib/src/cow_http2_machine.erl', line: 1199]},
    {:cowboy_http2, :maybe_send_data, 4, [file: '/Users/tony/repo/tony612/grpc-elixir/interop/deps/cowboy/src/cowboy_http2.erl', line: 770]},
    {:cowboy_http2, :commands, 3, [file: '/Users/tony/repo/tony612/grpc-elixir/interop/deps/cowboy/src/cowboy_http2.erl', line: 634]},
    {:cowboy_http2, :loop, 2, [file: '/Users/tony/repo/tony612/grpc-elixir/interop/deps/cowboy/src/cowboy_http2.erl', line: 247]},
    {:cowboy_http, :parse, 2, [file: '/Users/tony/repo/tony612/grpc-elixir/interop/deps/cowboy/src/cowboy_http.erl', line: 311]},
    {:proc_lib, :init_p_do_apply, 3, [file: 'proc_lib.erl', line: 249]}
  ]
}
essen commented 4 years ago

Yeah it should not enter this clause when it's trailers, so it's missing a condition:

https://github.com/ninenines/cowlib/blob/master/src/cow_http2_machine.erl#L1197

tony612 commented 4 years ago

I printed the window sizes:

StreamWindow: 0, ConnWindow: 46275 MinSendSize: 16384, SendSize: 388623
essen commented 4 years ago

Sizes don't matter when it's trailers.

tony612 commented 4 years ago

But from these code https://github.com/ninenines/cowlib/blob/master/src/cow_http2_machine.erl#L1187-L1191

SendSize = BufferSize + case DataOrFileOrTrailers of
        {data, D} -> iolist_size(D);
        #sendfile{bytes=B} -> B;
        {trailers, _} -> 0
    end,

When it's a trailers frame, the SendSize is BufferSize + 0, so if there are data frames in queue, the current SendSize will still be large. I guess that's why I enter this clause?

essen commented 4 years ago

It's just there so it doesn't crash. The if needs to not enter the clause if sending trailers, because they are queued or sent here: https://github.com/ninenines/cowlib/blob/master/src/cow_http2_machine.erl#L1291

tony612 commented 4 years ago

I didn't get it. The fact is the wrong clause is entered. Maybe we should change https://github.com/ninenines/cowlib/blob/master/src/cow_http2_machine.erl#L1187-L1191 from

SendSize = BufferSize + case DataOrFileOrTrailers of
        {data, D} -> iolist_size(D);
        #sendfile{bytes=B} -> B;
        {trailers, _} -> 0
    end,

to

    SendSize = case DataOrFileOrTrailers of
        {data, D} -> BufferSize + iolist_size(D);
        #sendfile{bytes=B} -> BufferSize + B;
        {trailers, _} -> 0
    end,
essen commented 4 years ago

if (element(1, DataOrFileOrTrailers) =/= trailers) andalso (StreamWindow < MinSendSize) andalso ...

tony612 commented 4 years ago

Yeah. This is indeed a solution.

essen commented 4 years ago

Anyway a test is necessary.

tony612 commented 4 years ago

How to add a test for in cowlib? I didn't see test cases like that in cowboy. Is there a reference/guide?

essen commented 4 years ago

Probably somewhere around here, just gotta adjust sizes and/or timings I suppose? Maybe stream_body_large should stream a larger body. https://github.com/ninenines/cowboy/blob/master/test/req_SUITE.erl#L1061

essen commented 4 years ago

Thanks!