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

Reduce chances of race conditions when processing deleted toots #9815

Merged
merged 2 commits into from
Jan 16, 2019

Conversation

ClearlyClaire
Copy link
Contributor

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.

@ClearlyClaire
Copy link
Contributor Author

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.


RedisLock.acquire(lock_options) do |lock|
if lock.acquired?
return if delete_arrived_first?(object_uri)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

@Gargron Gargron merged commit a4f07ba into mastodon:master Jan 16, 2019
@ClearlyClaire ClearlyClaire deleted the fixes/delete-processing branch March 14, 2019 15:46
hiyuki2578 pushed a commit to ProjectMyosotis/mastodon that referenced this pull request Oct 2, 2019
…odon#9815)

* Reduce chances of race conditions when processing deleted toots

* Prevent race condition when processing deleted toots
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