From 334af8efdcae2e38a47c5f572e4225f7cf04de1e Mon Sep 17 00:00:00 2001 From: Wojtek Mach Date: Tue, 14 Nov 2023 14:29:20 +0100 Subject: [PATCH 1/6] Deprecate :protocol in favour of :protocols --- lib/finch.ex | 36 +++++++++++++++++++-- lib/finch/http1/conn.ex | 3 +- lib/finch/http1/pool.ex | 2 +- lib/finch/pool_manager.ex | 16 ++++----- test/finch/http1/integration_proxy_test.exs | 2 +- test/finch/http1/integration_test.exs | 6 ++-- test/finch/http1/pool_test.exs | 4 +-- test/finch/http1/telemetry_test.exs | 2 +- test/finch/http2/integration_test.exs | 12 +++---- test/finch/http2/pool_test.exs | 2 +- test/finch/http2/telemetry_test.exs | 2 +- test/finch_test.exs | 16 ++++----- 12 files changed, 66 insertions(+), 37 deletions(-) diff --git a/lib/finch.ex b/lib/finch.ex index 10b72342..b528b8b9 100644 --- a/lib/finch.ex +++ b/lib/finch.ex @@ -18,8 +18,18 @@ defmodule Finch do @pool_config_schema [ protocol: [ type: {:in, [:http2, :http1]}, + deprecated: "Use :protocols instead." + ], + protocols: [ + type: + {:in, + [ + [:http1], + [:http2], + [:http1, :http2] + ]}, doc: "The type of connection and pool to use.", - default: :http1 + default: [:http1] ], size: [ type: :pos_integer, @@ -238,12 +248,34 @@ defmodule Finch do conn_opts |> Keyword.put(:ssl_key_log_file_device, ssl_key_log_file_device) |> Keyword.put(:transport_opts, transport_opts) + |> Keyword.put(:protocols, valid[:protocols]) + + mod = + case valid[:protocol] do + :http1 -> + Finch.HTTP1.Pool + + :http2 -> + Finch.HTTP2.Pool + + nil -> + case valid[:protocols] do + [:http1] -> + Finch.HTTP1.Pool + + [:http2] -> + Finch.HTTP2.Pool + + [:http1, :http2] -> + Finch.HTTP1.Pool + end + end %{ + mod: mod, size: valid[:size], count: valid[:count], conn_opts: conn_opts, - protocol: valid[:protocol], conn_max_idle_time: to_native(valid[:max_idle_time] || valid[:conn_max_idle_time]), pool_max_idle_time: valid[:pool_max_idle_time] } diff --git a/lib/finch/http1/conn.ex b/lib/finch/http1/conn.ex index 14df344c..ff203b63 100644 --- a/lib/finch/http1/conn.ex +++ b/lib/finch/http1/conn.ex @@ -1,4 +1,4 @@ -defmodule Finch.Conn do +defmodule Finch.HTTP1.Conn do @moduledoc false alias Finch.SSL @@ -41,6 +41,7 @@ defmodule Finch.Conn do # custom protocols in case they don't know if a connection # is HTTP1/HTTP2, but they are fine as treating HTTP2 # connections has HTTP2. + conn_opts = conn.opts |> Keyword.put(:mode, :passive) diff --git a/lib/finch/http1/pool.ex b/lib/finch/http1/pool.ex index f9a23816..c9541938 100644 --- a/lib/finch/http1/pool.ex +++ b/lib/finch/http1/pool.ex @@ -3,7 +3,7 @@ defmodule Finch.HTTP1.Pool do @behaviour NimblePool @behaviour Finch.Pool - alias Finch.Conn + alias Finch.HTTP1.Conn alias Finch.Telemetry def child_spec(opts) do diff --git a/lib/finch/pool_manager.ex b/lib/finch/pool_manager.ex index ac32909d..b2564ee9 100644 --- a/lib/finch/pool_manager.ex +++ b/lib/finch/pool_manager.ex @@ -72,15 +72,14 @@ defmodule Finch.PoolManager do defp do_start_pools(shp, config) do pool_config = pool_config(config, shp) - pool_args = pool_args(shp, config, pool_config) - pool_mod = pool_mod(pool_config.protocol) - Enum.map(1..pool_config.count, fn _ -> # Choose pool type here... - {:ok, pid} = DynamicSupervisor.start_child(config.supervisor_name, {pool_mod, pool_args}) - {pid, pool_mod} + {:ok, pid} = + DynamicSupervisor.start_child(config.supervisor_name, {pool_config.mod, pool_args}) + + {pid, pool_config.mod} end) |> hd() end @@ -118,12 +117,9 @@ defmodule Finch.PoolManager do defp maybe_add_hostname(config, _), do: config - defp pool_mod(:http1), do: Finch.HTTP1.Pool - defp pool_mod(:http2), do: Finch.HTTP2.Pool - - defp pool_args(shp, config, %{protocol: :http1} = pool_config), + defp pool_args(shp, config, %{mod: Finch.HTTP1.Pool} = pool_config), do: {shp, config.registry_name, pool_config.size, pool_config, pool_config.pool_max_idle_time} - defp pool_args(shp, config, %{protocol: :http2} = pool_config), + defp pool_args(shp, config, %{mod: Finch.HTTP2.Pool} = pool_config), do: {shp, config.registry_name, pool_config.size, pool_config} end diff --git a/test/finch/http1/integration_proxy_test.exs b/test/finch/http1/integration_proxy_test.exs index fbfeb96b..ff273879 100644 --- a/test/finch/http1/integration_proxy_test.exs +++ b/test/finch/http1/integration_proxy_test.exs @@ -24,7 +24,7 @@ defmodule Finch.HTTP1.IntegrationProxyTest do name: ProxyFinch, pools: %{ default: [ - protocol: :http1, + protocols: [:http1], conn_opts: [ proxy: {:http, "localhost", proxy_port, []} ] diff --git a/test/finch/http1/integration_test.exs b/test/finch/http1/integration_test.exs index 8fa2dcfd..29399ba1 100644 --- a/test/finch/http1/integration_test.exs +++ b/test/finch/http1/integration_test.exs @@ -22,7 +22,7 @@ defmodule Finch.HTTP1.IntegrationTest do name: H2Finch, pools: %{ default: [ - protocol: :http2, + protocols: [:http2], conn_opts: [ transport_opts: [ verify: :verify_none @@ -114,7 +114,7 @@ defmodule Finch.HTTP1.IntegrationTest do name: H1Finch, pools: %{ default: [ - protocol: :http1 + protocols: [:http1] ] }} ) @@ -132,7 +132,7 @@ defmodule Finch.HTTP1.IntegrationTest do name: H1Finch, pools: %{ default: [ - protocol: :http1, + protocols: [:http1], conn_opts: [ transport_opts: [ reuse_sessions: false, diff --git a/test/finch/http1/pool_test.exs b/test/finch/http1/pool_test.exs index 88951f2c..d01241cc 100644 --- a/test/finch/http1/pool_test.exs +++ b/test/finch/http1/pool_test.exs @@ -32,7 +32,7 @@ defmodule Finch.HTTP1.PoolTest do name: IdleFinch, pools: %{ default: [ - protocol: :http1, + protocols: [:http1], pool_max_idle_time: 5 ] }} @@ -63,7 +63,7 @@ defmodule Finch.HTTP1.PoolTest do @describetag bypass: false setup %{finch_name: finch_name} do - start_supervised!({Finch, name: finch_name, pools: %{default: [protocol: :http1]}}) + start_supervised!({Finch, name: finch_name, pools: %{default: [protocols: [:http1]]}}) :ok end diff --git a/test/finch/http1/telemetry_test.exs b/test/finch/http1/telemetry_test.exs index 00b62b6a..fd0ae10e 100644 --- a/test/finch/http1/telemetry_test.exs +++ b/test/finch/http1/telemetry_test.exs @@ -9,7 +9,7 @@ defmodule Finch.HTTP1.TelemetryTest do end) start_supervised!( - {Finch, name: finch_name, pools: %{default: [protocol: :http1, conn_max_idle_time: 10]}} + {Finch, name: finch_name, pools: %{default: [protocols: [:http1], conn_max_idle_time: 10]}} ) :ok diff --git a/test/finch/http2/integration_test.exs b/test/finch/http2/integration_test.exs index 0d31541f..eede6806 100644 --- a/test/finch/http2/integration_test.exs +++ b/test/finch/http2/integration_test.exs @@ -20,7 +20,7 @@ defmodule Finch.HTTP2.IntegrationTest do name: TestFinch, pools: %{ default: [ - protocol: :http2, + protocols: [:http2], count: 5, conn_opts: [ transport_opts: [ @@ -41,7 +41,7 @@ defmodule Finch.HTTP2.IntegrationTest do name: TestFinch, pools: %{ default: [ - protocol: :http2, + protocols: [:http2], count: 5, conn_opts: [ transport_opts: [ @@ -65,7 +65,7 @@ defmodule Finch.HTTP2.IntegrationTest do name: TestFinch, pools: %{ default: [ - protocol: :http2, + protocols: [:http2], count: 1, conn_opts: [ transport_opts: [ @@ -109,7 +109,7 @@ defmodule Finch.HTTP2.IntegrationTest do name: TestFinch, pools: %{ default: [ - protocol: :http2, + protocols: [:http2], count: 5, conn_opts: [ transport_opts: [ @@ -138,7 +138,7 @@ defmodule Finch.HTTP2.IntegrationTest do name: TestFinch, pools: %{ default: [ - protocol: :http2, + protocols: [:http2], conn_opts: [ transport_opts: [ verify: :verify_none @@ -168,7 +168,7 @@ defmodule Finch.HTTP2.IntegrationTest do name: TestFinch, pools: %{ default: [ - protocol: :http2, + protocols: [:http2], conn_opts: [ transport_opts: [ verify: :verify_none diff --git a/test/finch/http2/pool_test.exs b/test/finch/http2/pool_test.exs index e6212dc3..39e9eae9 100644 --- a/test/finch/http2/pool_test.exs +++ b/test/finch/http2/pool_test.exs @@ -457,7 +457,7 @@ defmodule Finch.HTTP2.PoolTest do name: finch_name, pools: %{ default: [ - protocol: :http2, + protocols: [:http2], count: 5, conn_opts: [ transport_opts: [ diff --git a/test/finch/http2/telemetry_test.exs b/test/finch/http2/telemetry_test.exs index bd9afe23..74451f7b 100644 --- a/test/finch/http2/telemetry_test.exs +++ b/test/finch/http2/telemetry_test.exs @@ -9,7 +9,7 @@ defmodule Finch.HTTP2.TelemetryTest do end) start_supervised!( - {Finch, name: finch_name, pools: %{default: [protocol: :http2, conn_max_idle_time: 10]}} + {Finch, name: finch_name, pools: %{default: [protocols: [:http2], conn_max_idle_time: 10]}} ) :ok diff --git a/test/finch_test.exs b/test/finch_test.exs index 8c14d2dd..6727759e 100644 --- a/test/finch_test.exs +++ b/test/finch_test.exs @@ -278,7 +278,7 @@ defmodule FinchTest do name: finch_name, pools: %{ default: [ - protocol: :http2, + protocols: [:http2], count: 1, conn_opts: [ transport_opts: [ @@ -329,7 +329,7 @@ defmodule FinchTest do name: finch_name, pools: %{ default: [ - protocol: :http2, + protocols: [:http2], count: 1, conn_opts: [ transport_opts: [ @@ -379,7 +379,7 @@ defmodule FinchTest do name: finch_name, pools: %{ default: [ - protocol: :http2, + protocols: [:http2], count: 1, conn_opts: [ transport_opts: [ @@ -601,7 +601,7 @@ defmodule FinchTest do test "are passed through to the conn", %{bypass: bypass} do expect_any(bypass) - start_supervised!({Finch, name: H1Finch, pools: %{default: [protocol: :http1]}}) + start_supervised!({Finch, name: H1Finch, pools: %{default: [protocols: [:http1]]}}) assert {:ok, _} = Finch.build(:get, endpoint(bypass)) |> Finch.request(H1Finch) @@ -660,7 +660,7 @@ defmodule FinchTest do name: finch_name, pools: %{ default: [ - protocol: :http2, + protocols: [:http2], count: 1, conn_opts: [ transport_opts: [ @@ -839,7 +839,7 @@ defmodule FinchTest do name: finch_name, pools: %{ default: [ - protocol: :http2 + protocols: [:http2] ] }} ) @@ -867,7 +867,7 @@ defmodule FinchTest do name: finch_name, pools: %{ default: [ - protocol: :http2 + protocols: [:http2] ] }} ) @@ -915,7 +915,7 @@ defmodule FinchTest do name: finch_name, pools: %{ default: [ - protocol: :http2 + protocols: [:http2] ] }} ) From 9821049188cb42b09238bfea4562340b861d3669 Mon Sep 17 00:00:00 2001 From: Wojtek Mach Date: Tue, 14 Nov 2023 17:55:16 +0100 Subject: [PATCH 2/6] Update finch.ex --- lib/finch.ex | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lib/finch.ex b/lib/finch.ex index b528b8b9..e758d2d6 100644 --- a/lib/finch.ex +++ b/lib/finch.ex @@ -65,10 +65,7 @@ defmodule Finch do doc: """ These options are passed to `Mint.HTTP.connect/4` whenever a new connection is established. \ `:mode` is not configurable as Finch must control this setting. Typically these options are \ - used to configure proxying, https settings, or connect timeouts. \ - - For HTTP1 pools, the pool will force HTTP1 connections by default but you can perform ALPN \ - over HTTP1 pools by setting the `:protocols` option. + used to configure proxying, https settings, or connect timeouts. """, default: [] ], From 508721f22022f2b8e536cbbc85825d1b0c2a8646 Mon Sep 17 00:00:00 2001 From: Wojtek Mach Date: Tue, 14 Nov 2023 17:56:09 +0100 Subject: [PATCH 3/6] Apply suggestions from code review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: José Valim --- lib/finch.ex | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/lib/finch.ex b/lib/finch.ex index e758d2d6..9557c261 100644 --- a/lib/finch.ex +++ b/lib/finch.ex @@ -21,14 +21,15 @@ defmodule Finch do deprecated: "Use :protocols instead." ], protocols: [ - type: - {:in, - [ - [:http1], - [:http2], - [:http1, :http2] - ]}, - doc: "The type of connection and pool to use.", + type: {:list, {:in, [:http1, :http2]}}, + doc: """ + The type of connections to support. + + If using `:http1` only, an HTTP1 pool without multiplexing is used. \ + If using `:http2` only, an HTTP2 pool with multiplexing is used. \ + If both are listed, then both HTTP1/HTTP2 connections are \ + supported (via ALPN), but there is no multiplexing. + """, default: [:http1] ], size: [ From c5a123ab4c1acac770fa77f9b7bff752f0845acf Mon Sep 17 00:00:00 2001 From: Wojtek Mach Date: Tue, 14 Nov 2023 17:57:39 +0100 Subject: [PATCH 4/6] Update lib/finch.ex --- lib/finch.ex | 13 ++++--------- 1 file changed, 4 insertions(+), 9 deletions(-) diff --git a/lib/finch.ex b/lib/finch.ex index 9557c261..a223e05c 100644 --- a/lib/finch.ex +++ b/lib/finch.ex @@ -257,15 +257,10 @@ defmodule Finch do Finch.HTTP2.Pool nil -> - case valid[:protocols] do - [:http1] -> - Finch.HTTP1.Pool - - [:http2] -> - Finch.HTTP2.Pool - - [:http1, :http2] -> - Finch.HTTP1.Pool + if :http1 in valid[:protocols] do + Finch.HTTP1.Pool + else + Finch.HTTP2.Pool end end From 37247f784a4e56b172dc182ce4cc3ea491de5dde Mon Sep 17 00:00:00 2001 From: Wojtek Mach Date: Tue, 14 Nov 2023 17:58:57 +0100 Subject: [PATCH 5/6] Update lib/finch.ex --- lib/finch.ex | 1 + 1 file changed, 1 insertion(+) diff --git a/lib/finch.ex b/lib/finch.ex index a223e05c..e24df5d1 100644 --- a/lib/finch.ex +++ b/lib/finch.ex @@ -249,6 +249,7 @@ defmodule Finch do |> Keyword.put(:protocols, valid[:protocols]) mod = + # TODO: Remove :protocol on v0.18 case valid[:protocol] do :http1 -> Finch.HTTP1.Pool From 2927fb6b14d7290f6a3b819f9d9bdf078db7d3cc Mon Sep 17 00:00:00 2001 From: Wojtek Mach Date: Tue, 14 Nov 2023 18:08:32 +0100 Subject: [PATCH 6/6] up --- lib/finch.ex | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/finch.ex b/lib/finch.ex index e24df5d1..186a320f 100644 --- a/lib/finch.ex +++ b/lib/finch.ex @@ -248,8 +248,8 @@ defmodule Finch do |> Keyword.put(:transport_opts, transport_opts) |> Keyword.put(:protocols, valid[:protocols]) + # TODO: Remove :protocol on v0.18 mod = - # TODO: Remove :protocol on v0.18 case valid[:protocol] do :http1 -> Finch.HTTP1.Pool