New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cowboy 2 support #604

Merged
merged 10 commits into from Oct 26, 2017

Conversation

Projects
None yet
4 participants
@Gazler
Member

Gazler commented Oct 10, 2017

Not ready for merging yet. Have the following things to do:

  • Convert compress option to automatically use the correct stream handler VoiceLayer/plug_cowboy2#3 (comment)
  • Support Elixir 1.5 child_spec
  • Add a stream handler for cookies that are too long 4d4c48e
  • Support num_acceptors in some way (it is no longer passed to Cowboy)
  • check translator
  • investigate using stream handler
  • Fix intermittent test failures

Initial support for #599

Gazler added some commits Oct 10, 2017

Copy Cowboy Adapter to Cowboy2
In order to make it easier to see the changes required for Cowboy2, the
Cowboy adapter has been copied over and used as a base.
Allow cowboy version to be toggled via lockfile
There are now 2 lockfiles that can be used for testing. The default
`mix.lock` is used for cowboy 2.0 and above. If there is a
COWBOY_VERSION environment variable set that starts with a 1 then the
`mix-cowboy1.lock` file is used.

The tests have been updated to ignore the tests for the other version
via tags.

@Gazler Gazler changed the title from WIP: Cowboy 2 support to Cowboy 2 support Oct 17, 2017

@Gazler

This comment has been minimized.

Show comment
Hide comment
@Gazler

Gazler Oct 17, 2017

Member

@josevalim I think this is ready for review. There are intermittent failures on Cowboy2 which I haven't diagnosed yet though. When merging this, it'd be best not to squash so we can keep the commit history for this.

Member

Gazler commented Oct 17, 2017

@josevalim I think this is ready for review. There are intermittent failures on Cowboy2 which I haven't diagnosed yet though. When merging this, it'd be best not to squash so we can keep the commit history for this.

Defaults to 5000ms.
* `:protocol_options` - Specifies remaining protocol options,
see [Cowboy2 protocol docs](http://ninenines.eu/docs/en/cowboy/2.0/manual/cowboy_protocol/).

This comment has been minimized.

@AndrewDryga

AndrewDryga Oct 18, 2017

Contributor

The link is broken.

@AndrewDryga

AndrewDryga Oct 18, 2017

Contributor

The link is broken.

This comment has been minimized.

@AndrewDryga

AndrewDryga Oct 18, 2017

Contributor

Cowboy docs for v2.0 states that many options are shared and I can't find a separate cowboy_protocol document for v2.0.

@AndrewDryga

AndrewDryga Oct 18, 2017

Contributor

Cowboy docs for v2.0 states that many options are shared and I can't find a separate cowboy_protocol document for v2.0.

This comment has been minimized.

@Gazler
@Gazler

Gazler Oct 18, 2017

Member
defp otp_app(cowboy_options) do
if app = cowboy_options[:otp_app] do
Application.app_dir(app)

This comment has been minimized.

@AndrewDryga

AndrewDryga Oct 18, 2017

Contributor

Probably this is not optimal way to fech priv_dir path, see here: https://elixirforum.com/t/elixir-blog-posts/150/185.

@AndrewDryga

AndrewDryga Oct 18, 2017

Contributor

Probably this is not optimal way to fech priv_dir path, see here: https://elixirforum.com/t/elixir-blog-posts/150/185.

This comment has been minimized.

@josevalim

josevalim Oct 18, 2017

Member

It is fine for our use case though. :)

@josevalim

josevalim Oct 18, 2017

Member

It is fine for our use case though. :)

@AndrewDryga

This comment has been minimized.

Show comment
Hide comment
@AndrewDryga

AndrewDryga Oct 18, 2017

Contributor

@Gazler I think it would be nice to run code formatter on this PR, so nobody would need to do that later.

Contributor

AndrewDryga commented Oct 18, 2017

@Gazler I think it would be nice to run code formatter on this PR, so nobody would need to do that later.

Show outdated Hide outdated lib/plug/adapters/cowboy2.ex Outdated
@@ -0,0 +1,52 @@
defmodule Plug.Adapters.Cowboy2.Handler do

This comment has been minimized.

@josevalim

josevalim Oct 18, 2017

Member

Can we implement the early response check here instead of adding a custom handler? Maybe we cannot.

@josevalim

josevalim Oct 18, 2017

Member

Can we implement the early response check here instead of adding a custom handler? Maybe we cannot.

Show outdated Hide outdated lib/plug/adapters/cowboy2.ex Outdated
Show outdated Hide outdated lib/plug/adapters/cowboy2.ex Outdated

Gazler added some commits Oct 10, 2017

Adapter for Cowboy2
There are the following general changes:

 * the child_spec is defined manually instead of using ranch.child_spec
   to allow either `cowboy_http` or `cowboy_http2` to be used
 * the `acceptors` option is passed to ranch as a transport option
 * the protocol options are now a map instead of a keyword list
    * all headers are now transformed to a map instead of a list of pairs
Convert compress option to stream_handler for Cowboy2 adapter
In order to maintain backwards compatability, if the compress option is
set then the `cowboy_compress_h` handler is used. This is how
compression is configured in Cowboy 2. This is not compatible with the
users setting their own stream_handlers.

This was requested in VoiceLayer/plug_cowboy2#3 (comment)
Use handler to check for large headers in Cowboy 2
The Plug.Adapters.Cowboy2.BadResponseCheck stream_handler is
automatically added both with and without compress being set.
Add translate support for cowboy 2
The module to use is determined based on the version of cowboy that is currently
running.
@Gazler

This comment has been minimized.

Show comment
Hide comment
@Gazler

Gazler Oct 20, 2017

Member

@josevalim could you review the last 2 commits and make sure that you are happy with them?

Member

Gazler commented Oct 20, 2017

@josevalim could you review the last 2 commits and make sure that you are happy with them?

Show outdated Hide outdated lib/plug/adapters/cowboy2.ex Outdated
case {compress, stream_handlers} do
{true, nil} ->
Keyword.put_new(cowboy_options, :stream_handlers, [

This comment has been minimized.

@josevalim

josevalim Oct 25, 2017

Member

Maybe move those handlers to a @default_stream_handlers?

@josevalim

josevalim Oct 25, 2017

Member

Maybe move those handlers to a @default_stream_handlers?

Show outdated Hide outdated lib/plug/adapters/cowboy2.ex Outdated
Show outdated Hide outdated lib/plug/adapters/cowboy2.ex Outdated
Show outdated Hide outdated lib/plug/adapters/cowboy2.ex Outdated
Show outdated Hide outdated lib/plug/adapters/cowboy2.ex Outdated
end
def send_file(req, status, headers, path, offset, length) do
%File.Stat{type: :regular, size: size} = File.stat!(path)

This comment has been minimized.

@josevalim

josevalim Oct 25, 2017

Member

Shouldn't we read the file only if length is :all? Maybe even something like:

def send_file(req, status, headers, path, offset, :all) do
  %File.Stat{type: :regular, size: size} = File.stat!(path)
  send_file(req, status, headers, path, offset, size)
end

Although I guess we are using File.stat! as a check if the file exists...

@josevalim

josevalim Oct 25, 2017

Member

Shouldn't we read the file only if length is :all? Maybe even something like:

def send_file(req, status, headers, path, offset, :all) do
  %File.Stat{type: :regular, size: size} = File.stat!(path)
  send_file(req, status, headers, path, offset, size)
end

Although I guess we are using File.stat! as a check if the file exists...

This comment has been minimized.

@Gazler

Gazler Oct 26, 2017

Member

This is how it works with the Cowboy adapter, so I'm going to leave it for now.

@Gazler

Gazler Oct 26, 2017

Member

This is how it works with the Cowboy adapter, so I'm going to leave it for now.

case System.get_env("COWBOY_VERSION") do
"1" <> _ -> "mix-cowboy1.lock"
_ -> "mix.lock"
end

This comment has been minimized.

@josevalim

josevalim Oct 25, 2017

Member

👍

@josevalim

josevalim Oct 25, 2017

Member

👍

@Gazler Gazler merged commit 7378e84 into master Oct 26, 2017

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Oct 26, 2017

Member

❤️ 💚 💙 💛 💜

Member

josevalim commented Oct 26, 2017

❤️ 💚 💙 💛 💜

@drozdzynski

This comment has been minimized.

Show comment
Hide comment
@drozdzynski

drozdzynski Oct 28, 2017

When new release of plug with this pull request?

drozdzynski commented Oct 28, 2017

When new release of plug with this pull request?

@josevalim

This comment has been minimized.

Show comment
Hide comment
@josevalim

josevalim Oct 28, 2017

Member

It is up to @Gazler :) We probably need to do other tasks before, for example, make sure Phoenix master runs comfortable on this new Plug version and can leverage its HTTP2 support.

Member

josevalim commented Oct 28, 2017

It is up to @Gazler :) We probably need to do other tasks before, for example, make sure Phoenix master runs comfortable on this new Plug version and can leverage its HTTP2 support.

@Gazler Gazler deleted the gr-cowboy2 branch Oct 29, 2017

Gazler added a commit to Gazler/plug that referenced this pull request Oct 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment