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

Reduce chances of race conditions when processing deleted toots #9815

Merged
merged 2 commits into from Jan 16, 2019

Conversation

Projects
None yet
2 participants
@ThibG
Copy link
Collaborator

ThibG commented Jan 15, 2019

I have noticed that sometimes, toots are not correctly deleted across instances (despite a direct Follow relationship).

One of those cases may be because of a race condition between the processing of Create and Delete when a toot is quickly created then deleted.

This PR reduces chances of such a race condition happening, but does not eliminate it. It should be possible to avoid this race condition completely by moving the delete_arrived_first? check into the locked block, and lock the delete_later! call as well, but I'm not sure about the performances impact.

@ThibG ThibG force-pushed the ThibG:fixes/delete-processing branch from 85365c8 to 80f8e6a Jan 15, 2019

@ThibG

This comment has been minimized.

Copy link
Collaborator Author

ThibG commented Jan 15, 2019

Added a commit which uses a lock, ensuring that a deleted status has either been processed or won't be processed at all. I am not too sure about the performance implications when processing Delete.

@ThibG ThibG force-pushed the ThibG:fixes/delete-processing branch from 80f8e6a to 71f4193 Jan 15, 2019


RedisLock.acquire(lock_options) do |lock|
if lock.acquired?
return if delete_arrived_first?(object_uri)

This comment has been minimized.

@ThibG

ThibG Jan 16, 2019

Author Collaborator

CodeClimate complains, but I find it clearer to just return here. Or maybe there's something I don't understand about Ruby blocks?

This comment has been minimized.

@Gargron

Gargron Jan 16, 2019

Member

I believe the code does what you want it to do, but in most cases return from a block is a mistake.

@@ -21,11 +21,13 @@ def delete_person
def delete_note
return if object_uri.nil?

RedisLock.acquire(lock_options) do |lock|
delete_later!(object_uri)

This comment has been minimized.

@ThibG

ThibG Jan 16, 2019

Author Collaborator

CodeClimate complains about us not using lock, but we can't leave it out.
We don't use it because we want to call delete_later! even in the unlikely case where we couldn't acquire the lock, it's not really an issue.

This comment has been minimized.

@Gargron

Gargron Jan 16, 2019

Member

We can totally leave it out though? Or just name it _lock

This comment has been minimized.

@ThibG

ThibG Jan 16, 2019

Author Collaborator

We can't leave it out because Redislock checks for that parameter (see https://circleci.com/gh/tootsuite/mastodon/39022?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link). I will name it _lock.

@ThibG ThibG force-pushed the ThibG:fixes/delete-processing branch from 71f4193 to 285a38f Jan 16, 2019

@ThibG ThibG force-pushed the ThibG:fixes/delete-processing branch from 285a38f to 2fc1ac3 Jan 16, 2019

@Gargron Gargron merged commit a4f07ba into tootsuite:master Jan 16, 2019

11 checks passed

ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: check-i18n Your tests passed on CircleCI!
Details
ci/circleci: install Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: install-ruby2.6 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.4 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.5 Your tests passed on CircleCI!
Details
ci/circleci: test-ruby2.6 Your tests passed on CircleCI!
Details
ci/circleci: test-webui Your tests passed on CircleCI!
Details
codeclimate All good!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment