HTTP/1 request with empty body and content-type header leaves connection in bad state

[petrohi]

On subsequent request:

{:error,
 {:function_clause,
   [{:gun_http, :request,
     [{:http_state, #PID<0.200.0>,
       {:sslsocket, {:gen_tcp, #Port<0.6542>, :tls_connection, :undefined},
        #PID<0.247.0>}, :ranch_ssl, :"HTTP/1.1", [:gun_data], :keepalive, "",
       [], :head, {0, 0}, :body_chunked,
       #Function<0.21809937/1 in :gun_http.init/4>},
      #Reference<0.291734004.3684958210.98171>, #PID<0.200.0>, "PUT",
      'api.xyz.com', 443, "/some",
      [{"content-type", "application/json"}]],
     [file: '/Users/phizalev/machine_gun/deps/gun/src/gun_http.erl',
      line: 281]},
    {:gun, :loop, 1,
     [file: '/Users/phizalev/machine_gun/deps/gun/src/gun.erl', line: 683]},
    {:gun, :proc_lib_hack, 5,
     [file: '/Users/phizalev/machine_gun/deps/gun/src/gun.erl', line: 545]},
    {:proc_lib, :init_p_do_apply, 3, [file: 'proc_lib.erl', line: 247]}]}}

[essen]

What do you mean by empty body? How do you call it?

[petrohi]

Like this: :gun.request(pid, "PUT", "/some", [{"content-type", "application/json"}], "", %{})

[essen]

That's not a bug per se, perhaps the interface is not obvious. If you set content-type but don't set a body you are expected to use the gun:data functions to send that body.

[petrohi]

Would probably make sense for body be some atom in this case. But yes--makes sense.

[essen]

Yes I don't disagree.

[essen]

Let's keep it open if you don't mind, it'd be good to fix this, or at least error out earlier, at some point.

[essen]

https://github.com/petrohi/machine_gun/issues/1 is an example real-world issue caused by this.

Also occurs for gun:get/2.

[essen]

It's going to be hard to rework this in a backward compatible manner but let's give it a try.

• delete/2,3,4: fin, body has no meaning
• get/2,3,4: fin, body has no meaning
• head/2,3,4: fin, body has no meaning
• options/2,3,4: fin, body has no meaning
• patch/3, post/3, put/3: check headers to decide, nofin if content-length/content-type, fin otherwise
• patch/4,5, post/4,5, put/4,5: fin, body is provided as an argument
• request/4: check headers to decide, nofin if content-length/content-type, fin otherwise
• request/5,6: fin, body is provided as an argument

I suggest having two functions instead of request/4,5,6 in the future:

• headers/4,5 which sends headers and expects more data to come separately
• request/5,6 which sends headers and body and expects no more data to come

That means that the method-functions can be converted to calling one or the other function depending on whether they expect more data afterwards.

The following can be deprecated and removed in Gun 2.0:

• request/4 deprecated in favor of headers/4,5
• content-length/content-type header check deprecated, in Gun 2.0 the functions patch/3, post/3 and put/3 will always expect a body to follow separately; if one needs to send a PATCH/POST/PUT without a body they can always call the functions request/5,6

I think this can be done without breaking any user code, since the documented behavior will be preserved. Thoughts?

[essen]

Edited a proposal. Thoughts?

[petrohi]

Should this be?

patch/4,5, post/4,5, put/4,5: nofin, body is provided as an argument

[essen]

No by nofin I mean the data function can be called (the body isn't finished after the request call), while fin sends everything it gets and terminates the request.

[petrohi]

Understand now. request/4 with headers check is temporary for compatibility?

[essen]

Yes until Gun 2.0. request/4 would be removed then and you'd have to call one or the other function to be able to send body or not. So we'd still have the header issue for patch/3, post/3, put/3 and request/4 for now but request/4 would be undocumented immediately in favor of the functions headers/4,5, and it's a lesser problem for the other 3.

[9mm]

So how do i need to deal with this? currently I have an API returning 80K req/min with "" body and no content-length

[9mm]

PS i should mention I'm using machine_gun

[essen]

I'm currently doing some rework and tbh will probably make it Gun 2.0 and do the breaking changes we need.

That said, I'm not sure you have the right bug, if you do then all you need to do is remove the content-type or ensure you send a final empty data.

[essen]

Please review 630bd47 which contains the proposed solution for Gun 2.0.

[essen]

I'll close this meanwhile since I believe this is addressed but feel free to say otherwise or reopen. Thanks!