In issue #2948 a crash was reported in processCommand(). Later Oran Agra
(@oranagra) traced the bug (in private chat) in the following sequence
of events:
1. Some maxmemory is set.
2. The slave is the currently active client and is executing PING or
REPLCONF or whatever a slave can send to its master.
3. freeMemoryIfNeeded() is called since maxmemory is set.
4. flushSlavesOutputBuffers() is called by freeMemoryIfNeeded().
5. During slaves buffers flush, a write error could be encoutered in
writeToClient() or sendReplyToClient() depending on the version of
Redis. This will trigger freeClient() against the currently active
client, so a segmentation fault will likely happen in
processCommand() immediately after the call to freeMemoryIfNeeded().
There are different possible fixes:
1. Add flags to writeToClient() (recent versions code base) so that
we can ignore the write errors, and use this flag in
flushSlavesOutputBuffers(). However this is not simple to do in older
versions of Redis.
2. Use freeClientAsync() during write errors. This works but changes the
current behavior of releasing clients ASAP when possible. Normally
we write to clients during the normal event loop processing, in the
writable client, where there is no active client, so no care must be
taken.
3. The fix of this commit: to detect that the current client is no
longer valid. This fix is a bit "ad-hoc", but works across all the
versions and has the advantage of not changing the remaining
behavior. Only alters what happens during this race condition,
hopefully.
My guess was that wait3() with WNOHANG could never return -1 and an
error. However issue #2897 may possibly indicate that this could happen
under non clear conditions. While we try to understand this better,
better to handle a return value of -1 explicitly, otherwise in the
case a BGREWRITE is in progress but wait3() returns -1, the effect is to
match the first branch of the if/else block since server.rdb_child_pid
is -1, and call backgroundSaveDoneHandler() without a good reason, that
will, in turn, crash the Redis server with an assertion.
Maybe there are legitimate use cases for MIGRATE inside Lua scripts, at
least for now. When the command will be executed in an asynchronous
fashion (planned) it is possible we'll no longer be able to permit it
from within Lua scripts.
Thanks to Oran Agra (@oranagra) for reporting. Key extraction would not
work otherwise and it does not make sense to take wrong data in the
command table.
Currently this feature is only accessible via DEBUG for testing, since
otherwise depending on the instance configuration a given script works
or is broken, which is against the Redis philosophy.
By calling redis.replicate_commands(), the scripting engine of Redis
switches to commands replication instead of replicating whole scripts.
This is useful when the script execution is costly but only results in a
few writes performed to the dataset.
Morover, in this mode, it is possible to call functions with side
effects freely, since the script execution does not need to be
deterministic: anyway we'll capture the outcome from the point of view
of changes to the dataset.
In this mode math.random() returns different sequences at every call.
If redis.replicate_commnads() is not called before any other write, the
command returns false and sticks to whole scripts replication instead.
This new function is able to restart the server "in place". The current
Redis process executes the same executable it was executed with, using
the same arguments and configuration file.
