Allow collect_body/3 to be called just once about plug HOT 9 CLOSED

@josevalim I'll look into that. Also, we need to s the non-deprecated functions from cowboy_req for reading the body. Currently we are using stream_body/2 which will be removed in 1.0. The new method for this is cowboy_req:body/2.

https://github.com/extend/cowboy/blob/0.10.0/src/cowboy_req.erl#L99

I'll work on updating this as well.

from plug.

@scrogson When you are adding support for cowboy 0.10.0 can you make the timeout configurable when collecting the body? Timeouts were added to the new functions for reading the body in cowboy. We really need that feature in hex :)

from plug.

After discussing with @josevalim in chat, we've decided to rename this to read_body/2. The signature is very simple:

def read_body(conn, opts \\ []), do: ... end

@ericmj the method will just delegate to cowboy_req:body/2 which will give you full control over the options passed.

Cowboy allows: length, read_length, read_timeout, transfer_decode (fn), and content_decode (fn) as options.

I'm currently working through a test failure and should have this in a PR in the next couple of days.

from plug.

Should we just pass the options through directly instead of making them part of the adapter interface?

from plug.

@ericmj not sure I understand what you mean. I'm passing the options directly into the adapter. As I understand it, we are reading the req_body from the adapters state.

def read_body(%Conn{adapter: {adapter, state}} = conn, opts \\ []) do
  case adapter.read_req_body(state, opts) do
    {:ok, data, state} ->
      {:ok, data, %{conn | adapter: {adapter, state}}}
    {:more, data, state} ->
      {:more, data, %{conn | adapter: {adapter, state}}}
  end
end

from plug.

@scrogson Plug is supposed to be generic for any adapter. Today we only have cowboy so the issue won't come up. But what if we add another webserver adapter that instead of :read_timeout uses :timeout as the name for the timeout option? We should define and document the options in Plug so that they work for all possible adapters.

from plug.

Yeah, I was talking to @scrogson and we have decided exactly that. We are going to use the same names as cowboy, but all supported options should be explicitly documented. We should never say "accepts the same options as cowboy".

from plug.

@josevalim @scrogson Awesome! 👍

from plug.

I've sent a PR for this work in #44

@josevalim and I have discussed that returning {:error, :already_collected, conn} is not really necessary.

from plug.