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

Update only_one to use flock (advisory lock) instead of pidfile #35225

Merged
merged 2 commits into from Jun 9, 2020

Conversation

wjordan
Copy link
Contributor

@wjordan wjordan commented Jun 9, 2020

The implementation of only_one.rb (#only_one_running?) reads a pidfile and (if it exists) sends a kill 0 to see if a previously-started process is still running. If not, the current process writes its PID to that file and deletes it with an at_exit hook on shutdown.

This script had two (known remaining) issues, both related to the at_exit hook deleting the pidfile on shutdown:

  1. Forked processes also call the at_exit hook when they exit, causing the file to be deleted before the process actually exited;
  2. A process can hang in between the at_exit hook and the process actually shutting down, causing the file to be deleted before the process actually exited.

These issues could be dealt with by fixing them individually (see #35069 for a fix to 1), or by removing the at_exit entirely and depending only on the kill 0 rather than the file-existence to determine if another process is still running.

This PR offers an approach using advisory file locks (see flock(2)) via File#flock, which ends up being simpler than the approach using pidfiles and kill 0.

Testing story

We didn't have existing test coverage for this script, so I added a simple test OnlyOneTest covering basic behavior using fork that fails on staging but passes with either this PR or #35069.

Reviewer Checklist:

  • Tests provide adequate coverage
  • Code is well-commented
  • New features are translatable or updates will not break translations
  • Relevant documentation has been added or updated
  • User impact is well-understood and desirable
  • Pull Request is labeled appropriately
  • Follow-up work items (including potential tech debt) are tracked and linked

Copy link
Contributor

@Hamms Hamms left a comment

Choose a reason for hiding this comment

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

This approach looks great! Love how close we're able to get to a bare-metal bash solution here. Love the test, too!

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.

None yet

2 participants