Skip to content

Async worker: reconnect correctly after redis connection errors#255

Merged
bors[bot] merged 1 commit intomasterfrom
worker-async-reconnect-on-redis-errors
Feb 5, 2021
Merged

Async worker: reconnect correctly after redis connection errors#255
bors[bot] merged 1 commit intomasterfrom
worker-async-reconnect-on-redis-errors

Conversation

@davidor
Copy link
Copy Markdown
Contributor

@davidor davidor commented Feb 5, 2021

This PR fixes a bug: the async worker was not reconnecting correctly after redis connection errors.

The reason is that it was instantiating a client without the "resque" namespace.

The client was not namespaced, so the worker was trying to fetch jobs
from the incorrect queues after recovering from a redis connection
error.
@davidor davidor requested a review from unleashed February 5, 2021 12:05
Comment thread spec/integration/worker_async_spec.rb Outdated
Comment on lines +104 to +105
worker.shutdown
worker_thread.join
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hm, I think you could just force MRI to kill the thread here if it did not finish in time to make sure we won't wait too much for whatever reason (ie. actually having some sort of connectivity problem, such as having the server respond very slowly), because as it stands we'll raise just fine, but we'll have to wait.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried it without applying the fix and it worked fine.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Writing a correct integration test for this is complicated because of some limitations of the testing suite that require some work to be fixed.
I'll merge this now because we need the fix to unblock other work, I'll try to come back to this next week.

@davidor
Copy link
Copy Markdown
Contributor Author

davidor commented Feb 5, 2021

bors r=@unleashed

@bors
Copy link
Copy Markdown
Contributor

bors Bot commented Feb 5, 2021

🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

@bors
Copy link
Copy Markdown
Contributor

bors Bot commented Feb 5, 2021

👎 Rejected by PR status

@davidor davidor force-pushed the worker-async-reconnect-on-redis-errors branch from d37f72a to 4f73a1c Compare February 5, 2021 15:46
@davidor
Copy link
Copy Markdown
Contributor Author

davidor commented Feb 5, 2021

bors retry

@bors
Copy link
Copy Markdown
Contributor

bors Bot commented Feb 5, 2021

Build succeeded:

@bors bors Bot merged commit b46ec09 into master Feb 5, 2021
@bors bors Bot deleted the worker-async-reconnect-on-redis-errors branch February 5, 2021 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants