Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OnConflict::Replace: yield when lock was achieved #640

Merged
merged 4 commits into from
Sep 27, 2021

Conversation

mhenrixon
Copy link
Owner

@mhenrixon mhenrixon commented Sep 27, 2021

Close #629

The replace strategy creates a new lock after deleting the old lock and the old job. After a lock has been achieved we need to allow the worker to yield in that specific scenario.

@dsander can you check if this solves the problem for you?

Close #629

The problem originally was that in some situations, a string was returned (for the lock). This should never happen, any conflict strategy must return nil to avoid pretending to be successful.
@dsander
Copy link
Contributor

dsander commented Sep 27, 2021

Thanks for looking into it! Sadly it doesn't work, you can pull in the integration tests I added from this commit projectivetech@23dc652.

When using on_conflict: :replace the middleware can not return nil because that tells sidekiq not to enqueue the new job. An approach similar to :reschedule might work, but then the caller of perform_async still would not get the newly enqueued jid and kind of break the Sidekiq API.

This should be considered a success
@mhenrixon
Copy link
Owner Author

@dsander the tests you provided passes in this branch now

@mhenrixon mhenrixon changed the title OnConflict: return nil OnConflict::Replace: yield when lock was achieved Sep 27, 2021
@mhenrixon mhenrixon force-pushed the on-conflict-return-nil branch from c5ec9ed to 59b1c14 Compare September 27, 2021 12:28
@mhenrixon mhenrixon merged commit d4914fb into main Sep 27, 2021
@delete-merged-branch delete-merged-branch bot deleted the on-conflict-return-nil branch September 27, 2021 12:30
@dsander
Copy link
Contributor

dsander commented Sep 28, 2021

Thank you for fixing it!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants