From 6ff1268228601aaa532e48eaea285681c3ae1e90 Mon Sep 17 00:00:00 2001 From: Noemi <45180344+unflxw@users.noreply.github.com> Date: Wed, 6 Nov 2024 12:06:55 +0100 Subject: [PATCH 1/2] Use separate Hackney pool for transmitter This relates to the issue reported in #970, where the default Hackney pool is drained of connections by the check-ins sent by the transmitter. This does not fix the issue, but ensures that it is a separate pool, used only by the AppSignal transmitter, that is drained of connections, rather than the default one, which may be used by the customer's code. --- lib/appsignal.ex | 3 ++- lib/appsignal/transmitter.ex | 7 +++++++ test/appsignal/transmitter_test.exs | 13 +++++++++---- 3 files changed, 18 insertions(+), 5 deletions(-) diff --git a/lib/appsignal.ex b/lib/appsignal.ex index 79dff003..38878a7a 100644 --- a/lib/appsignal.ex +++ b/lib/appsignal.ex @@ -48,7 +48,8 @@ defmodule Appsignal do {Appsignal.Tracer, []}, {Appsignal.Monitor, []}, {Appsignal.Probes, []}, - {Appsignal.CheckIn.Scheduler, []} + {Appsignal.CheckIn.Scheduler, []}, + :hackney_pool.child_spec(:appsignal_transmitter, []) ] result = Supervisor.start_link(children, strategy: :one_for_one, name: Appsignal.Supervisor) diff --git a/lib/appsignal/transmitter.ex b/lib/appsignal/transmitter.ex index f03e708c..b8374bbb 100644 --- a/lib/appsignal/transmitter.ex +++ b/lib/appsignal/transmitter.ex @@ -41,6 +41,13 @@ defmodule Appsignal.Transmitter do end defp options do + ssl_options() ++ + [ + pool: :appsignal_transmitter + ] + end + + defp ssl_options do ca_file_path = Appsignal.Config.ca_file_path() options = diff --git a/test/appsignal/transmitter_test.exs b/test/appsignal/transmitter_test.exs index 7ed1a6fc..fd960462 100644 --- a/test/appsignal/transmitter_test.exs +++ b/test/appsignal/transmitter_test.exs @@ -80,9 +80,11 @@ defmodule Appsignal.TransmitterTest do end test "uses the default CA certificate" do - [_method, _url, _headers, _body, [ssl_options: ssl_options]] = + [_method, _url, _headers, _body, options] = Transmitter.request(:get, "https://example.com") + ssl_options = Keyword.get(options, :ssl_options) + assert ssl_options[:verify] == :verify_peer assert ssl_options[:cacertfile] == Config.ca_file_path() @@ -123,9 +125,10 @@ defmodule Appsignal.TransmitterTest do path = "priv/cacert.pem" with_config(%{ca_file_path: path}, fn -> - [_method, _url, _headers, _body, [ssl_options: ssl_options]] = + [_method, _url, _headers, _body, options] = Transmitter.request(:get, "https://example.com") + ssl_options = Keyword.get(options, :ssl_options) assert ssl_options[:cacertfile] == path end) end @@ -136,8 +139,10 @@ defmodule Appsignal.TransmitterTest do with_config(%{ca_file_path: path}, fn -> log = capture_log(fn -> - assert [_method, _url, _headers, _body, []] = - Transmitter.request(:get, "https://example.com") + [_method, _url, _headers, _body, options] = + Transmitter.request(:get, "https://example.com") + + refute Keyword.has_key?(options, :ssl_options) end) # credo:disable-for-lines:2 Credo.Check.Readability.MaxLineLength From 7233adfa3b7a5fc16e523f00bbd85a42e8cbc148 Mon Sep 17 00:00:00 2001 From: Noemi <45180344+unflxw@users.noreply.github.com> Date: Wed, 6 Nov 2024 12:32:03 +0100 Subject: [PATCH 2/2] Close Hackney connections after use Make sure that Hackney connections are closed after use. This fixes an issue where the Hackney pool does not correctly claim back connections, which fixes #970. Implement a `transmit_and_close/3` convenience method in the transmitter for use cases where the body is not of interest, meaning that the connection can be immediately closed. --- ...-check-ins-not-being-sent-after-some-time.md | 8 ++++++++ lib/appsignal/check_in/scheduler.ex | 6 +++--- lib/appsignal/diagnose/report.ex | 2 ++ lib/appsignal/transmitter.ex | 17 +++++++++++++++++ lib/appsignal/utils/push_api_key_validator.ex | 8 ++++---- test/support/appsignal/fake_transmitter.ex | 10 ++++++++++ 6 files changed, 44 insertions(+), 7 deletions(-) create mode 100644 .changesets/fix-check-ins-not-being-sent-after-some-time.md diff --git a/.changesets/fix-check-ins-not-being-sent-after-some-time.md b/.changesets/fix-check-ins-not-being-sent-after-some-time.md new file mode 100644 index 00000000..c35e762f --- /dev/null +++ b/.changesets/fix-check-ins-not-being-sent-after-some-time.md @@ -0,0 +1,8 @@ +--- +bump: patch +type: fix +--- + +Fix an issue where, after a certain amount of time, check-ins would no longer be sent. + +This issue also caused the default Hackney connection pool to be saturated, affecting other code that uses the default Hackney connection pool. diff --git a/lib/appsignal/check_in/scheduler.ex b/lib/appsignal/check_in/scheduler.ex index dc7bb55a..f28000b7 100644 --- a/lib/appsignal/check_in/scheduler.ex +++ b/lib/appsignal/check_in/scheduler.ex @@ -98,11 +98,11 @@ defmodule Appsignal.CheckIn.Scheduler do config = Appsignal.Config.config() endpoint = "#{config[:logging_endpoint]}/check_ins/json" - case @transmitter.transmit(endpoint, {Enum.reverse(events), :ndjson}, config) do - {:ok, status_code, _, _} when status_code in 200..299 -> + case @transmitter.transmit_and_close(endpoint, {Enum.reverse(events), :ndjson}, config) do + {:ok, status_code, _} when status_code in 200..299 -> @integration_logger.trace("Transmitted #{description}") - {:ok, status_code, _, _} -> + {:ok, status_code, _} -> @integration_logger.error( "Failed to transmit #{description}: status code was #{status_code}" ) diff --git a/lib/appsignal/diagnose/report.ex b/lib/appsignal/diagnose/report.ex index 7b64c90a..bc8ecb8c 100644 --- a/lib/appsignal/diagnose/report.ex +++ b/lib/appsignal/diagnose/report.ex @@ -13,6 +13,7 @@ defmodule Appsignal.Diagnose.Report do case Transmitter.transmit(config[:diagnose_endpoint], {%{diagnose: report}, :json}, config) do {:ok, 200, _, reference} -> {:ok, body} = :hackney.body(reference) + :hackney.close(reference) case Jason.decode(body) do {:ok, response} -> {:ok, response["token"]} @@ -21,6 +22,7 @@ defmodule Appsignal.Diagnose.Report do {:ok, status_code, _, reference} -> {:ok, body} = :hackney.body(reference) + :hackney.close(reference) {:error, %{status_code: status_code, body: body}} {:error, reason} -> diff --git a/lib/appsignal/transmitter.ex b/lib/appsignal/transmitter.ex index b8374bbb..dc0e0a48 100644 --- a/lib/appsignal/transmitter.ex +++ b/lib/appsignal/transmitter.ex @@ -13,6 +13,12 @@ defmodule Appsignal.Transmitter do def transmit(url, payload_and_format \\ {nil, nil}, config \\ nil) def transmit(url, nil, config), do: transmit(url, {nil, nil}, config) + # This function calls `:hackney.request/5` -- it is the + # caller's responsibility to ensure that `:hackney.close/1` is + # called afterwards. + # + # If you're not interested in the body, only in the status code + # and headers, use `transmit_and_close/3` instead. def transmit(url, {payload, format}, config) do config = config || Appsignal.Config.config() @@ -32,6 +38,17 @@ defmodule Appsignal.Transmitter do request(:post, url, headers, body) end + def transmit_and_close(url, payload_and_format \\ {nil, nil}, config \\ nil) do + case transmit(url, payload_and_format, config) do + {:ok, status, headers, reference} -> + :hackney.close(reference) + {:ok, status, headers} + + {:error, reason} -> + {:error, reason} + end + end + defp encode_body(nil, _), do: "" defp encode_body(payload, :json), do: Jason.encode!(payload) diff --git a/lib/appsignal/utils/push_api_key_validator.ex b/lib/appsignal/utils/push_api_key_validator.ex index ba43c115..3f884b7b 100644 --- a/lib/appsignal/utils/push_api_key_validator.ex +++ b/lib/appsignal/utils/push_api_key_validator.ex @@ -5,10 +5,10 @@ defmodule Appsignal.Utils.PushApiKeyValidator do def validate(config) do url = "#{config[:endpoint]}/1/auth" - case Transmitter.transmit(url, nil, config) do - {:ok, 200, _, _} -> :ok - {:ok, 401, _, _} -> {:error, :invalid} - {:ok, status_code, _, _} -> {:error, status_code} + case Transmitter.transmit_and_close(url, nil, config) do + {:ok, 200, _} -> :ok + {:ok, 401, _} -> {:error, :invalid} + {:ok, status_code, _} -> {:error, status_code} {:error, reason} -> {:error, reason} end end diff --git a/test/support/appsignal/fake_transmitter.ex b/test/support/appsignal/fake_transmitter.ex index 81e334d8..a4b8a040 100644 --- a/test/support/appsignal/fake_transmitter.ex +++ b/test/support/appsignal/fake_transmitter.ex @@ -31,6 +31,16 @@ defmodule Appsignal.FakeTransmitter do Agent.get(__MODULE__, & &1[:response]).() end + def transmit_and_close(url, payload, config) do + case transmit(url, payload, config) do + {:ok, status, headers, _reference} -> + {:ok, status, headers} + + {:error, reason} -> + {:error, reason} + end + end + def transmitted do Agent.get(__MODULE__, &Enum.reverse(&1[:transmitted])) end