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

Add a db lock system to prevent race conditions #104

Merged
merged 35 commits into from
Feb 21, 2024

Conversation

RasmusSkytte
Copy link
Contributor

@RasmusSkytte RasmusSkytte commented Feb 6, 2024

Intent

Fixes #63

Approach

This PR adds the db locking system developed for diseasystore and expands and improves it.

The original description from diseasystore:

Four functions were written to implement flock-like locking for data base tables.
add_table_lock() a function that assigns the current process id (pid) to a given table
remove_table_lock() a function that removes the current process id (pid) from a given table
is_lock_owner() a function that returns TRUE if the current process id (pid) is assigned to a given table
remove_expired_locks() a function that removes old locks.

These locks are stored in a "locks" table in a given schema (set by the schema argument in the function calls).
Each entry is also identified by a timestamp for the lock creation "lock_start", which is used to detect stale locks.

Going from there, the remove_expired_locks() was removed and instead we throw an error in is_lock_owner() if there is a stale lock on the table.
For example, if a process crashes mid update_snapshot() we may want to not release the lock and instead create a "rollback" method to fix the botched update.

Tests were added for the functions which led to a number of fixes to the functions.

Finally, a simple implementation was made to update_snapshot where it attempts to gain lock on the table before starting the update.

Known issues

This PR also contains the code being merged in

Once merged, this PR will be rebased and opened.

Tests for PostgreSQL are showing as failing.
This is caused by an update to the Workflows so that previously undetected errors are now being explicitly shown.
The source of the error is being fixed in #98

Checklist

  • The PR passes all local unit tests
  • I have documented any new features introduced
  • If the PR adds a new feature, please add an entry in NEWS.md
  • A reviewer is assigned to this PR

@RasmusSkytte RasmusSkytte linked an issue Feb 6, 2024 that may be closed by this pull request
@RasmusSkytte RasmusSkytte self-assigned this Feb 6, 2024
@RasmusSkytte RasmusSkytte marked this pull request as draft February 6, 2024 13:43
@RasmusSkytte RasmusSkytte force-pushed the feature/63-race-condition-prevention branch 4 times, most recently from cdde0f1 to a5514c9 Compare February 9, 2024 13:21
@RasmusSkytte RasmusSkytte marked this pull request as ready for review February 9, 2024 13:34
@RasmusSkytte RasmusSkytte requested review from a team, kaare-gr and LasseEngboChr and removed request for a team February 9, 2024 13:34
@RasmusSkytte RasmusSkytte added this to the v0.4 milestone Feb 14, 2024
@RasmusSkytte RasmusSkytte added the enhancement New feature or request label Feb 14, 2024
@RasmusSkytte RasmusSkytte force-pushed the feature/63-race-condition-prevention branch from 4c283d3 to 6fb2455 Compare February 20, 2024 14:43
R/locks.R Outdated Show resolved Hide resolved
@LasseEngboChr
Copy link
Contributor

One could consider adding an easy way to get all expired locks. And as mentioned in an inline comment then adding a username may make it safer when manually removing locks.

@RasmusSkytte
Copy link
Contributor Author

I have refactored the three locking functions into two (lock_table and unlock_table).

lock_table returns is the merger of add_table_lock and is_lock_owner.

unlock_table is the old remove_table_lock but now also takes a pid argument to remove stale locks with different pid

R/locks.R Outdated Show resolved Hide resolved
Copy link
Contributor

@LasseEngboChr LasseEngboChr left a comment

Choose a reason for hiding this comment

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

Great simplification - I only have a couple of small suggestions

@RasmusSkytte RasmusSkytte merged commit 4c965b5 into main Feb 21, 2024
25 checks passed
@RasmusSkytte RasmusSkytte deleted the feature/63-race-condition-prevention branch February 21, 2024 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

FEATURE: Tools for race condition prevention
2 participants