ninenines / gun

HTTP/1.1, HTTP/2, Websocket client (and more) for Erlang/OTP.
ISC License
898 stars 231 forks source link

HTTP/1 cancel causes subsequent crash #140

Closed petrohi closed 6 years ago

petrohi commented 6 years ago

Cancel seems to be broken with crash at :gun_http.handle_head when subsequent request is made on the same connection.

essen commented 6 years ago

Guess you missed my question. What's the crash, is it because streams=[] in your case?

petrohi commented 6 years ago

Here you go:

{:function_clause,
  [{:gun_http, :handle_head,
    ["HTTP/1.1 200 OK\r\nDate: Tue, 09 Jan 2018 22:43:46 GMT\r\nContent-Type: text/plain\r\nContent-Length: 2\r\nConnection: keep-alive\r\nserver: Cowboy\r\n\r\nOK",
     {:http_state, #PID<0.769.0>,
      {:sslsocket, {:gen_tcp, #Port<0.42895>, :tls_connection, :undefined},
       #PID<0.780.0>}, :ranch_ssl, :"HTTP/1.1", [:gun_data], :keepalive, "",
      [{#Reference<0.443231660.1382547461.79982>, false},
       {:stream, #Reference<0.443231660.1382547461.80020>, #PID<0.769.0>, "GET",
        true, :undefined}], :head, {0, 0}, :head,
      #Function<0.21809937/1 in :gun_http.init/4>}],
    [file: '/Users/peter/server/deps/gun/src/gun_http.erl', line: 197]},
   {:gun, :loop, 1,
    [file: '/Users/peter/server/deps/gun/src/gun.erl', line: 656]},
   {:gun, :proc_lib_hack, 5,
    [file: '/Users/peter/server/deps/gun/src/gun.erl', line: 545]},
   {:proc_lib, :init_p_do_apply, 3, [file: 'proc_lib.erl', line: 247]}]}
essen commented 6 years ago

Ah I see the stream somehow became a tuple instead of a record. I'll try to look into it tomorrow.

essen commented 6 years ago

Doesn't seem like canceling streams ever worked, or at least not in a long time. If you change the cancel_stream function to return Tuple#{is_alive=false}; instead of the tuple, does it fix your problem?

petrohi commented 6 years ago

Nope. Same crash. To make sure:

cancel_stream(State=#http_state{streams=Streams}, StreamRef) ->
    Streams2 = [case Ref of
        StreamRef ->
            {Ref, false};
        _ ->
            Tuple#stream{is_alive=false}
    end || Tuple = #stream{ref=Ref} <- Streams],
    State#http_state{streams=Streams2}.
essen commented 6 years ago

No on the first clause, sorry I forgot the variable is named Tuple. Replace the {Ref, false} tuple with it and leave the second clause as Tuple.

petrohi commented 6 years ago

Works now!

essen commented 6 years ago

Pushed a new tag 1.0.0-pre.5.

essen commented 6 years ago

I think next time we can do 1.0.0 I don't think the API will change in incompatible ways even if we introduce maps or improve the content-type request stuff.