Allow users to share a single redis connection across forks within a single worker

[isnotajoke]

Resque will (unless I'm missing something obvious) intentionally disconnect & reconnect its redis connection across forks. This is safe, but results in a lot of connection churn, which can become an issue in environments with a lot of tasks. The redis-rb library exposes a configuration option that allows parent and child processes to share a single redis connection as long as they don't both attempt to use it at the same time. Resque seems to guard against this, so using the option should be safe. This PR enables this by:

• Introducing a share_redis configuration option. If truthy, Resque will refrain from explicitly cycling its redis connection after a fork.
• Adding that option to the README
• Adding a test.

In local testing in our environment, this change works properly (there aren't any new errors in our jobs), and reduces our connection churn dramatically.

[oehlschl]

Thanks @isnotajoke! Came across this while also looking for Resque optimizations and it seems like it could be helpful for us as well.

I'm happy to pilot this branch for our jobs but want to make sure I'm not missing something super obvious. @rafaelfranca any thoughts as an expert?

Thanks again. I'll start playing around with this locally today.

[jeremy]

Think this can and probably should be the default, rather than something to turn on only for high traffic deployments.

How about flipping the flag and calling it something like Resque.reconnect_redis_per_job?, following the fork_per_job? language? Defaulting to false.

Let's explicitly check that inherit_socket is true in this case, too. (Redis client will raise InheritedError, but it's not clear what to fix in Resque configuration.)

[pieterza]

+1

[pieterza]

Any update on this one @jeremy ? I'm happy to have a go at it unless you feel like responding to the changes requested @isnotajoke

[isnotajoke]

(apologies for the radio silence; haven't had time to take another look at this)

The main reason I opted to not make it the default is that it's a change from previous behavior, and (without in depth knowledge of how Resque's releases are structured), I tend to prefer releasing new behavior as default off initially, changing it to on in subsequent releases if desired (and if there aren't significant bugs or other issues). Happy to defer to convention if we prefer default on here, though.

Good call on the inherit_socket check; will try to make that change this weekend.

[pieterza]

Any update on this one? :)

[jgao54]

Very interested in this feature as well.

[iloveitaly]

I'd love to have this feature merged. How can I help get it done?

[iloveitaly]

@isnotajoke could you expand more on:

This removes some of the safety provided by Resque's forking model

What risks are there? What things should the user look out for when using this option?

Also, do you know if this causes any issues with resque worker pools? I don't think detailing that out is a requirement for getting this merged, but if you happen to know, I think it would be great to add this in.

[isnotajoke]

I haven't observed any specific risks (or things to look out for).

Decoupling task executions from one another (by forking, by sharing relatively little between parent & child processes across the fork) makes Resque very tolerant of even very destructive failures within individual tasks; the individual task execution may fail, but the worker process will remain functional. Adding more shared state across forks (as this feature does when enabled) chips away at that safety. Here, I'd be worried about tasks failing or exiting in a way that leaves the underlying Redis connection in a weird state, potentially causing issues for the worker process as it looks for work & updates queue state in redis, as well as for individual job executions. We never saw that in our local testing, but it is a risk (and something to consider if turning this on for a mature system with a lot of existing Resque tasks).

I think the explanation above is a lot for a README, but I'll try to add a little bit more detail on the risk to this change.

[iloveitaly]

Which client libraries does it not work with? Maybe we could be more explicit here and mention we only recommend using this with redis-rb?

[isnotajoke]

I only tested with redis-rb. Agree that calling that out ("we're confident it works for redis-rb; you should test thoroughly with other libraries") is a good move.

[iloveitaly]

Couple comments on documenting operational usage, but code-wise this PR looks great.

[isnotajoke]

@iloveitaly thanks for the feedback. I'll make some updates based on your suggestions.

While we did briefly test this in our infrastructure, we ultimately addressed the underlying concern (connection churn) at an infrastructure level rather than with this patch. I still think the idea makes sense, but I'd be more confident in this PR if someone could vouch for it working in their environment. (I can help with some basic sanity checking in my environment, but can't commit to a specific date for having that done).