Skip to content

Commit

Permalink
Handle Redis servers that disable CLIENT command
Browse files Browse the repository at this point in the history
See #192.

Before this commit, we would not handle the case where the Redis server
had disabled CLIENT commands. In that case, when using noreply_*
functions (which issue CLIENT commands under the hood), we would mess up
the state of the ETS table used for queueing commands to reply to. That
would show up as a hard-to-debug ArgumentError because of an unhandled
:"$end_of_table" return value from :ets.first/1.

With this commit, we made a couple of fixes to:

  * Handle empty ETS request queues
  * Handle errors returned by "CLIENT REPLY OFF" in
    Redix.noreply_pipeline/3

We also added a new Docker Compose Redis instance to test against a
server that disables CLIENT commands.
  • Loading branch information
whatyouhide committed May 8, 2022
1 parent 6cc0e70 commit 10b3193
Show file tree
Hide file tree
Showing 6 changed files with 67 additions and 5 deletions.
7 changes: 7 additions & 0 deletions docker-compose.yml
Original file line number Diff line number Diff line change
Expand Up @@ -61,3 +61,10 @@ services:
- 6384:6384
sysctls:
net.core.somaxconn: 1024

base_with_disallowed_client_command:
build: ./test/docker/base_with_disallowed_client_command
ports:
- "6386:6379"
sysctls:
net.core.somaxconn: 1024
12 changes: 9 additions & 3 deletions lib/redix.ex
Original file line number Diff line number Diff line change
Expand Up @@ -523,9 +523,15 @@ defmodule Redix do
assert_valid_pipeline_commands(commands)
commands = [["CLIENT", "REPLY", "OFF"]] ++ commands ++ [["CLIENT", "REPLY", "ON"]]

# The "OK" response comes from the last "CLIENT REPLY ON".
with {:ok, ["OK"]} <- pipeline_without_checks(conn, commands, opts),
do: :ok
case pipeline_without_checks(conn, commands, opts) do
# The "OK" response comes from the last "CLIENT REPLY ON".
{:ok, ["OK"]} -> :ok
# This can happen if there's an error with the first "CLIENT REPLY OFF" command, like
# when the Redis server disabled CLIENT commands.
{:ok, [%Redix.Error{} = error]} -> {:error, error}
# Connection errors and such are bubbled up.
{:error, reason} -> {:error, reason}
end
end

@doc """
Expand Down
17 changes: 15 additions & 2 deletions lib/redix/socket_owner.ex
Original file line number Diff line number Diff line change
Expand Up @@ -110,8 +110,21 @@ defmodule Redix.SocketOwner do
end

defp peek_element_in_queue(queue_table, index) do
first_key = :ets.first(queue_table)
:ets.lookup_element(queue_table, first_key, index)
case :ets.first(queue_table) do
:"$end_of_table" ->
# We can blow up here because there is nothing we can do.
# See https://github.com/whatyouhide/redix/issues/192
raise """
failed to find an original command in the commands queue. This can happen, for example, \
when the Redis server you are using does not support CLIENT commands. Redix issues \
CLIENT commands under the hood when you use noreply_pipeline/3 and other noreply_* \
functions. If that's not what you are doing, you might have found a bug in Redix: \
please open an issue! https://github.com/whatyouhide/redix/issues
"""

first_key ->
:ets.lookup_element(queue_table, first_key, index)
end
end

defp take_first_in_queue(queue_table) do
Expand Down
5 changes: 5 additions & 0 deletions test/docker/base_with_disallowed_client_command/Dockerfile
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
FROM redis:6.0.0-alpine

COPY ./redis.conf redis.conf

CMD ["redis-server", "./redis.conf"]
1 change: 1 addition & 0 deletions test/docker/base_with_disallowed_client_command/redis.conf
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
rename-command CLIENT ""
30 changes: 30 additions & 0 deletions test/redix_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -545,6 +545,36 @@ defmodule RedixTest do
Redix.noreply_command!(conn, ["SET", "noreply_cmd_bang_mykey", "myvalue"], timeout: 0)
end
end

# Regression for https://github.com/whatyouhide/redix/issues/192
@tag :capture_log
test "throw a human-readable error when the server disabled CLIENT commands and users " <>
"of Redix ignore function results" do
{:ok, conn} = Redix.start_link(port: 6386)
Process.flag(:trap_exit, true)
key = Base.encode16(:crypto.strong_rand_bytes(6))

# Here we send a command with noreply. Redis returns an error here since CLIENT commands
# (issued under the hood) are disabled, but we are simulating a user ignoring the results
# of the noreply_command/2 calls. When issuing more normal commands, Redix has trouble
# finding the command in the ETS queue when popping the response and used to fail
# with a hard-to-understand error (see the issue linked above).
_ = Redix.noreply_command(conn, ["INCR", key])

assert {%RuntimeError{} = error, _stacktrace} = catch_exit(Redix.command!(conn, ["PING"]))

assert Exception.message(error) =~
"failed to find an original command in the commands queue"
end

# Regression for https://github.com/whatyouhide/redix/issues/192
test "return an error when the server disabled CLIENT commands" do
{:ok, conn} = Redix.start_link(port: 6386)
key = Base.encode16(:crypto.strong_rand_bytes(6))

assert {:error, %Redix.Error{} = error} = Redix.noreply_command(conn, ["INCR", key])
assert error.message =~ "unknown command `CLIENT`"
end
end

describe "timeouts and network errors" do
Expand Down

0 comments on commit 10b3193

Please sign in to comment.