search

phoenixframework / phoenix_pubsub_redis

The Redis PubSub adapter for the Phoenix framework
175 stars 66 forks source link

Redis disconnection can lead to app shut down #27

Closed shosti closed 7 years ago

shosti commented 7 years ago

Steps to reproduce:

  1. Make a new Phoenix app
  2. Use Phoenix.PubSub.Redis as the pubsub adapter (as per instructions)
  3. Run mix phoenix.gen.presence and add the appropriate supervisor
  4. Run mix phoenix.server
  5. Turn off Redis locally

(Exact dependency versions: https://gist.github.com/shosti/46c73d522cc2c58997ccc5d28bdfa629 + Redis 3.2.4.)

Expected results

I was expecting it to attempt to reconnect until Redis comes back online (which IIUC is basically analogous to the behavior of the PG2 adapter).

Actual results

Once a heartbeat message is broadcast, a string of errors leads to total app shutdown: https://gist.github.com/shosti/8c82beee34fd994b6e9fa53d9625cf4d .

It looks like the problem is an unhandled function clause, but it may just be a matter of adjusting restart policies (let me know if that's the case).

shosti commented 7 years ago

FWIW I played around with ignoring the :disconnected message (https://github.com/phoenixframework/phoenix_pubsub_redis/commit/a2df8080930f1c0849525e283c9e980bdf32202e), but the effect is still the same. The reason is here: Redis errors get translated directly to BroadcastErrors in the PubSub.

I'm not super familiar with how Phoenix PubSub works under the hood, but IIUC netsplits within a PG2 group don't lead to BroadcastErrors in that way. Does it make sense to treat a redis disconnection like a netsplit? If so maybe connection errors should be ignored (or handled in a way other than a BroadcastError)?