From 28b00d60161f9a9cd60a7e76050887b9b1511853 Mon Sep 17 00:00:00 2001 From: hiett Date: Fri, 29 Sep 2023 03:11:19 +0100 Subject: [PATCH 1/4] Begin work ensuring that the pool is correctly being destroyed on connection failures --- lib/srh/http/command_handler.ex | 8 ++++++++ lib/srh/redis/client.ex | 10 +++++++++- lib/srh/redis/client_registry.ex | 2 ++ 3 files changed, 19 insertions(+), 1 deletion(-) diff --git a/lib/srh/http/command_handler.ex b/lib/srh/http/command_handler.ex index d671643..5861a19 100644 --- a/lib/srh/http/command_handler.ex +++ b/lib/srh/http/command_handler.ex @@ -169,12 +169,20 @@ defmodule Srh.Http.CommandHandler do when is_number(max_connections) do case GenRegistry.lookup_or_start(Client, srh_id, [max_connections, connection_info]) do {:ok, pid} -> + IO.puts("SRH: Found client #{inspect(pid)}") + # Run the command case Client.find_worker(pid) |> ClientWorker.redis_command(command_array) do {:ok, res} -> {:ok, %{result: res}} + # Jedix connection error + {:error, %{reason: :closed} = error} -> + # Ensure that this pool is killed, but still pass the error up the chain for the response + Client.destroy_workers(pid) + decode_error(error, srh_id) + {:error, error} -> decode_error(error, srh_id) end diff --git a/lib/srh/redis/client.ex b/lib/srh/redis/client.ex index 337ccb6..3531f5b 100644 --- a/lib/srh/redis/client.ex +++ b/lib/srh/redis/client.ex @@ -35,6 +35,10 @@ defmodule Srh.Redis.Client do GenServer.cast(client, {:return_worker, pid}) end + def destroy_workers(client) do + GenServer.cast(client, {:destroy_workers}) + end + def handle_call({:find_worker}, _from, %{registry_pid: registry_pid} = state) when is_pid(registry_pid) do {:ok, worker} = ClientRegistry.find_worker(registry_pid) @@ -59,13 +63,17 @@ defmodule Srh.Redis.Client do {:noreply, state} end + def handle_cast({:destroy_workers}, state) do + ClientRegistry.destroy_workers(state.registry_pid) + {:stop, :normal, state} + end + def handle_cast(_msg, state) do {:noreply, state} end def handle_info(:idle_death, state) do ClientRegistry.destroy_workers(state.registry_pid) - {:stop, :normal, state} end diff --git a/lib/srh/redis/client_registry.ex b/lib/srh/redis/client_registry.ex index 9db78d3..342bbfe 100644 --- a/lib/srh/redis/client_registry.ex +++ b/lib/srh/redis/client_registry.ex @@ -33,6 +33,8 @@ defmodule Srh.Redis.ClientRegistry do end def destroy_workers(registry) do + # TODO: remove before shipping + IO.puts("DEBUG: Destroying workers") GenServer.cast(registry, {:destroy_workers}) end From fa073854e2b06a4e0e571d2245fe2277d527535e Mon Sep 17 00:00:00 2001 From: hiett Date: Fri, 29 Sep 2023 18:34:44 +0100 Subject: [PATCH 2/4] All 3 methods are now destroying pools on failures, need to test normal functionality then can remove logging --- lib/srh/http/command_handler.ex | 6 ++++++ lib/srh/redis/client_worker.ex | 1 + 2 files changed, 7 insertions(+) diff --git a/lib/srh/http/command_handler.ex b/lib/srh/http/command_handler.ex index 5861a19..aabdfd2 100644 --- a/lib/srh/http/command_handler.ex +++ b/lib/srh/http/command_handler.ex @@ -130,6 +130,12 @@ defmodule Srh.Http.CommandHandler do # Fire back the result here, because the initial Multi was successful result + {:error, %{reason: :closed} = error} -> + IO.puts("Transaction error: #{inspect(error)}") + # Ensure that this pool is killed, but still pass the error up the chain for the response + Client.destroy_workers(client_pid) + decode_error(error, srh_id) + {:error, error} -> decode_error(error, srh_id) end diff --git a/lib/srh/redis/client_worker.ex b/lib/srh/redis/client_worker.ex index 5ccb710..7a88c4a 100644 --- a/lib/srh/redis/client_worker.ex +++ b/lib/srh/redis/client_worker.ex @@ -18,6 +18,7 @@ defmodule Srh.Redis.ClientWorker do end def redis_command(worker, command_array) do + IO.puts("redis_command (for pid #{inspect(worker)}): #{inspect(command_array)}") GenServer.call(worker, {:redis_command, command_array}) end From 7edc4a0c8209ac3c0716c9e2a4524a1b2f0b39be Mon Sep 17 00:00:00 2001 From: hiett Date: Fri, 29 Sep 2023 20:58:05 +0100 Subject: [PATCH 3/4] Remove old logs --- lib/srh/http/command_handler.ex | 3 --- lib/srh/redis/client_registry.ex | 2 -- lib/srh/redis/client_worker.ex | 1 - 3 files changed, 6 deletions(-) diff --git a/lib/srh/http/command_handler.ex b/lib/srh/http/command_handler.ex index aabdfd2..719dbab 100644 --- a/lib/srh/http/command_handler.ex +++ b/lib/srh/http/command_handler.ex @@ -131,7 +131,6 @@ defmodule Srh.Http.CommandHandler do result {:error, %{reason: :closed} = error} -> - IO.puts("Transaction error: #{inspect(error)}") # Ensure that this pool is killed, but still pass the error up the chain for the response Client.destroy_workers(client_pid) decode_error(error, srh_id) @@ -175,8 +174,6 @@ defmodule Srh.Http.CommandHandler do when is_number(max_connections) do case GenRegistry.lookup_or_start(Client, srh_id, [max_connections, connection_info]) do {:ok, pid} -> - IO.puts("SRH: Found client #{inspect(pid)}") - # Run the command case Client.find_worker(pid) |> ClientWorker.redis_command(command_array) do diff --git a/lib/srh/redis/client_registry.ex b/lib/srh/redis/client_registry.ex index 342bbfe..9db78d3 100644 --- a/lib/srh/redis/client_registry.ex +++ b/lib/srh/redis/client_registry.ex @@ -33,8 +33,6 @@ defmodule Srh.Redis.ClientRegistry do end def destroy_workers(registry) do - # TODO: remove before shipping - IO.puts("DEBUG: Destroying workers") GenServer.cast(registry, {:destroy_workers}) end diff --git a/lib/srh/redis/client_worker.ex b/lib/srh/redis/client_worker.ex index 7a88c4a..5ccb710 100644 --- a/lib/srh/redis/client_worker.ex +++ b/lib/srh/redis/client_worker.ex @@ -18,7 +18,6 @@ defmodule Srh.Redis.ClientWorker do end def redis_command(worker, command_array) do - IO.puts("redis_command (for pid #{inspect(worker)}): #{inspect(command_array)}") GenServer.call(worker, {:redis_command, command_array}) end From f085bb2eee51d8a70bf7ecd7f4f92945db5c2d6d Mon Sep 17 00:00:00 2001 From: hiett Date: Wed, 11 Oct 2023 16:18:49 +0100 Subject: [PATCH 4/4] SSL support --- lib/srh/http/base_router.ex | 5 ++++- lib/srh/http/content_type_check_plug.ex | 5 ++++- lib/srh/redis/client.ex | 2 +- lib/srh/redis/client_worker.ex | 22 +++++++++++++++++++++- 4 files changed, 30 insertions(+), 4 deletions(-) diff --git a/lib/srh/http/base_router.ex b/lib/srh/http/base_router.ex index 5763c7e..d788eff 100644 --- a/lib/srh/http/base_router.ex +++ b/lib/srh/http/base_router.ex @@ -26,7 +26,10 @@ defmodule Srh.Http.BaseRouter do end match _ do - handle_response({:not_found, "SRH: Endpoint not found. SRH might not support this feature yet."}, conn) + handle_response( + {:not_found, "SRH: Endpoint not found. SRH might not support this feature yet."}, + conn + ) end defp do_command_request(conn, success_lambda) do diff --git a/lib/srh/http/content_type_check_plug.ex b/lib/srh/http/content_type_check_plug.ex index 1352d57..868255b 100644 --- a/lib/srh/http/content_type_check_plug.ex +++ b/lib/srh/http/content_type_check_plug.ex @@ -35,7 +35,10 @@ defmodule Srh.Http.ContentTypeCheckPlug do # Return a custom error, ensuring the same format as the other errors conn |> put_resp_content_type("application/json") - |> send_resp(400, Jason.encode!(%{error: "Invalid content type. Expected application/json."})) + |> send_resp( + 400, + Jason.encode!(%{error: "Invalid content type. Expected application/json."}) + ) |> halt() end end diff --git a/lib/srh/redis/client.ex b/lib/srh/redis/client.ex index 3531f5b..3d1e39d 100644 --- a/lib/srh/redis/client.ex +++ b/lib/srh/redis/client.ex @@ -3,7 +3,7 @@ defmodule Srh.Redis.Client do alias Srh.Redis.ClientRegistry alias Srh.Redis.ClientWorker - @idle_death_time 1000 * 15 + @idle_death_time 1000 * 60 * 15 # 15 minutes def start_link(max_connections, connection_info) do GenServer.start_link(__MODULE__, {max_connections, connection_info}, []) diff --git a/lib/srh/redis/client_worker.ex b/lib/srh/redis/client_worker.ex index 5ccb710..d6bcca4 100644 --- a/lib/srh/redis/client_worker.ex +++ b/lib/srh/redis/client_worker.ex @@ -62,9 +62,29 @@ defmodule Srh.Redis.ClientWorker do } = state ) when is_binary(connection_string) do + enable_ssl = String.starts_with?(connection_string, "rediss://") + + socket_opts = + case enable_ssl do + true -> + [ + customize_hostname_check: [ + match_fun: :public_key.pkix_verify_hostname_match_fun(:https) + ] + ] + + false -> + [] + end + # NOTE: Redix only seems to open the connection when the first command is sent # This means that this will return :ok even if the connection string may not actually be connectable - {:ok, pid} = Redix.start_link(connection_string) + {:ok, pid} = + Redix.start_link(connection_string, + ssl: enable_ssl, + socket_opts: socket_opts + ) + {:noreply, %{state | redix_pid: pid}} end