Handle disconnected Redis connections

[shosti]

Here's my attempt at fixing #27. I'd love a sanity-check on the approach.

I tried this version with a local phoenix app in the following scenarios:

1. Start single server, turn redis off, wait a while, turn redis on
2. Start multiple servers, turn redis off, wait a while, turn redis on
3. Start multiple servers, turn redis off, do action on one server, turn redis on
4. Start multiple servers, turn redis off, kill a server, turn redis on

In all cases the presence state seemed to propagate correctly (and I didn't see any zombie presences as mentioned in #26).

The downside is that if there's a persistent redis issue, the app won't ever crash and state could diverge between nodes; I'm not sure if this solution is the right tradeoff.

[shosti]

@josevalim updated, seem better?

[josevalim]

@shosti yes! Although I am not a project maintainer, so someone else should also review and merge. :) Let's also ping @whatyouhide.

[mitchellhenke]

status field is from when we were using redo: https://github.com/phoenixframework/phoenix_pubsub_redis/pull/14

I'm not sure how much it applies to redix, as nothing in the application depends on it currently. People could potentially use it for debugging running applications though?

[josevalim]

@mitchellhenke well, maybe they can debug the redix connections directly then? If we are not using it, maybe we should ✂️ it. :)

[mitchellhenke]

@josevalim that was where I was leaning.

re:

The downside is that if there's a persistent redis issue, the app won't ever crash and state could diverge between nodes; I'm not sure if this solution is the right tradeoff.

Right now we attempt to reconnect without crashing on application start, so I would think it would be consistent to attempt to not crash on this as well.

[shosti]

I'd be happy to get rid of status as part of this as well.

[whatyouhide]

As far as the Redix side goes here, this looks good to me :)

[mitchellhenke]

@shosti that would be great

[shosti]

@mitchellhenke OK done.

[shosti]

Any other changes needed for this?

[chrismccord]

👍🏻

Sent from my iPhone

On Oct 26, 2016, at 6:09 AM, José Valim notifications@github.com wrote:

@josevalim commented on this pull request.

In lib/phoenix_pubsub_redis/redis_server.ex:

@@ -67,6 +70,11 @@ defmodule Phoenix.PubSub.RedisServer do {:noreply, state} end

• def handle_info({:redix_pubsub, redix_pid, :disconnected, %{reason: reason}}, %{redix_pid: redix_pid} = state) do
• Logger.error "disconnected from pubsub: #{reason}"
• {:noreply, state} Sorry to bikeshed but including the module is weird. And you can always have the module as part of the metadata. My suggestion:

Logger.error "Phoenix.PubSub disconnected from Redis with reason #{inspect reason} (awaiting reconnection)" — You are receiving this because you commented. Reply to this email directly, view it on GitHub, or mute the thread.

[shosti]

@josevalim @chrismccord OK I updated the error message. Anything else?

[mitchellhenke]

This looks good to me :)

[shosti]

Is this good to merge?

[mitchellhenke]

@shosti yep, thank you!