wooga / eredis

Erlang Redis client
MIT License
628 stars 280 forks source link

Race condition while reconnecting. #92

Closed ololoru closed 7 years ago

ololoru commented 8 years ago

I've discovered race condition while testing reconnection logic. Here below is setup I'm using:

1) Erlang (R16) is running on Macos, I start erlang shell and create new connection via eredis:start_link and do basic put/get test. I'm using default database, and no authentication 2) Redis is started in docker container. (I'm using native docker for macos) 3) I stop redis service in docker and see race condition results either in process crash or closed socket saved in eredis_client state.

When connection dies and socket receives tcp_closed message eredis_client:reconnect_loop is called in a separate process where it:

  1. Creates new socket
  2. Changes controlling process to original eredis_client PID
  3. Sends message to eredis_client PID to replace connection

There are the following issues with described approach:

ntrepid8 commented 7 years ago

I see this issue pop up when trying to connect to Redis version 3.0.6, but do not see it when connecting to Redis version 2.8.4.

edit: this turned out to be a different networking issue on my server running Redis 3.0.6, please disregard...

knutin commented 7 years ago

Oh, thats interesting. I'll try to have a look soon. Sorry for the delay in responding! On Sat, 11 Feb 2017 at 16:51, Josh Austin notifications@github.com wrote:

I see this issue pop up when trying to connect to Redis version 3.0.6, but do not see it when connecting to Redis version 2.8.4.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/wooga/eredis/issues/92#issuecomment-279159029, or mute the thread https://github.com/notifications/unsubscribe-auth/AAWPxFn1AH4YGDKZpfVbs0PmL0ctzs1kks5rbecTgaJpZM4KuC1N .

benbro commented 7 years ago

How is this different than any other socket failure? The socket can close in the following steps:

do_request(Req, From, State) returns an error when the socket is closed. The client can handle the error and retry. Isn't it enough?

Maybe we should handle the connection_ready message when the socket isn't undefined in the state?

benbro commented 7 years ago

Another option is to check for a live connection on every request and connect otherwise: https://github.com/interline/epgsql_pool/blob/master/src/epgsql_pool_worker.erl

knutin commented 7 years ago

Wouldn't that incur a significant performance penalty? On Tue 14. Mar 2017 at 09:41, benbro notifications@github.com wrote:

Another option is to check for a live connection on every request and connect otherwise:

https://github.com/interline/epgsql_pool/blob/master/src/epgsql_pool_worker.erl

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub https://github.com/wooga/eredis/issues/92#issuecomment-286482066, or mute the thread https://github.com/notifications/unsubscribe-auth/AAWPxDbLjTsYdc7PuOONBRjJ7HOIcycdks5rlsMkgaJpZM4KuC1N .