Skip to content

WIP: Switch locks to individual cache keys - #126

Closed
ethitter wants to merge 9 commits into
masterfrom
fix/lock-deadlock
Closed

ethitter wants to merge 9 commits into
masterfrom
fix/lock-deadlock

Conversation

@ethitter

@ethitter ethitter commented Aug 2, 2017

Copy link
Copy Markdown
Collaborator

Fixes #3

@ethitter ethitter added the wip label Aug 2, 2017
@ethitter ethitter self-assigned this Aug 2, 2017
@ethitter

ethitter commented Aug 2, 2017

Copy link
Copy Markdown
Collaborator Author

The multi-concurrency locks shouldn't be freed blindly as they are in 44c722f; it's a stepping stone.

@ethitter

ethitter commented Aug 5, 2017

Copy link
Copy Markdown
Collaborator Author

This was a silly approach. Instead of an array of locks, each numbered lock should be it's own cache. They expire on their own, and the class can handle some magic to make the many caches appear as a coherent lock. This has the added benefit of allowing for substitution of MySQL's GET_LOCK() or something Redis-backed, etc.

@WPprodigy

WPprodigy commented Nov 15, 2021 •

Copy link
Copy Markdown
Contributor

I think it would be good to combine check_single_lock/check_multi_lock type logic into one. Make it so the callers don't need to know or care if it's single or not.

Instead of an array of locks, each numbered lock should be it's own cache.

Definitely agree. While the array is better than the current situation where it's not able to really purge out abandoned locks, having all locks share a cache key leads to pretty unfortunate race conditions.

Letting locks have their own, and using wp_cache_add() even for further race-condition prevention, this will be much more sustainable. A "real" wp_cache_get_multiple() will also help a lot so it can pull them all at once.


For future readers, this will help solve the problem where a job is killed off (OOMs) higher up in the stack and is never able to release its lock. Since jobs will keep running and refreshing the lock timestamp, the "slot" of this OOM'd job will be forever taken until finally all slots are deadlocked and it can recognize that and wipe the all cache key out.

@WPprodigy WPprodigy changed the title Fix lock deadlock Switch locks to individual cache keys Nov 15, 2021
@WPprodigy WPprodigy changed the title Switch locks to individual cache keys WIP: Switch locks to individual cache keys Nov 15, 2021
@WPprodigy WPprodigy mentioned this pull request Nov 15, 2021
@WPprodigy

Copy link
Copy Markdown
Contributor

Closing this as it's a bit stale. Tracking in #233.

@WPprodigy WPprodigy closed this Dec 17, 2021
@WPprodigy
WPprodigy deleted the fix/lock-deadlock branch December 17, 2021 19:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Lock: Add tests for Lock class

2 participants