-
Notifications
You must be signed in to change notification settings - Fork 54
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 support for PID files #1849
Conversation
Robot Results
Passed Tests
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The pid must be written only once the lock acquired.
crates/common/flockfile/src/unix.rs
Outdated
// Write the PID to the lock file | ||
write(file.as_raw_fd(), pid_string.as_bytes()).map_err(|err| FlockfileError::FromNix { | ||
path: path.clone(), | ||
source: err, | ||
})?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is done too soon. The pid must be written to the file after the lock has been successfully acquired.
Otherwise, a concurrent process erases the pid of the lock owner.
$ tedge-mapper c8y >/dev/null 2>&1 &
[1] 188552
# The PID of the mapper is correctly set
$ cat /var/run/lock/tedge-mapper-c8y.lock
188552
# But if the a concurrent mapper is spawned
$ tedge-mapper c8y
2023-03-24T15:24:12.373980146Z ERROR flockfile::unix: Another instance of tedge-mapper-c8y is running.
2023-03-24T15:24:12.374092121Z ERROR flockfile::unix: Lock file path: /run/lock/tedge-mapper-c8y.lock
Error: Couldn't acquire file lock.
Caused by:
EAGAIN: Try again
# the concurrent process is stopped
# but the PID has been changed - unexpectedly
$ cat /var/run/lock/tedge-mapper-c8y.lock
188628%
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. I have got one question regarding this PR, there is also unistd::ftruncate
function offered by the nix
crate that allows to truncate the file to zero bytes before writing the PID. I am wondering whether there will be situations where the content of our lockfile may not be empty even though it should?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed. I have got one question regarding this PR, there is also
unistd::ftruncate
function offered by thenix
crate that allows to truncate the file to zero bytes before writing the PID. I am wondering whether there will be situations where the content of our lockfile may not be empty even though it should?
No this is useless as already done on open with the write option without the append option.
let path = NamedTempFile::new().unwrap().into_temp_path().to_owned(); | ||
let _ = Flockfile::new_lock(&path).unwrap(); | ||
|
||
let mut read_lockfile = OpenOptions::new().read(true).open(&path).unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For some reason I fail to spot, this line fails, the lockfile being not found;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes same for me, though when we will replace variable _
with _lockfile
it will work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, so _
drops the ignored value, while this is not the case for _lockfile
. Good to know.
Signed-off-by: Krzysztof Piotrowski <krzysztof.piotrowski@inetum.com>
c4d1420
to
df6ab8b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will approve after a chat with @reubenmiller to see if this can be merged or not for the 0.10 release.
let pid = Pid::from_raw(pid_string.parse().unwrap()); | ||
|
||
assert_eq!(pid, Pid::this()); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I feel it's better to add a test to check the behavior when the lock file creation is disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We agreed that no pid file is created when locking is disabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, thanks for the clarification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, minor comment :)
flock(file.as_raw_fd(), FlockArg::LockExclusiveNonblock).map_err(|err| { | ||
FlockfileError::FromNix { | ||
path: path.clone(), | ||
source: err, | ||
} | ||
})?; | ||
|
||
// Write the PID to the lock file | ||
write(file.as_raw_fd(), pid_string.as_bytes()).map_err(|err| FlockfileError::FromNix { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can combine the line 89 and this line together.
So remove this,
let pid_string = format!("{}", Pid::this());
Then,
write(file.as_raw_fd(), pid_string.as_bytes()).map_err(|err| FlockfileError::FromNix { | |
write(file.as_raw_fd(), Pid::this().to_string().as_bytes()).map_err(|err| FlockfileError::FromNix { |
Actually, if you want to have String
from a pure variable, using format!
is kinda abuse I think. .to_string()
is easier.
Proposed changes
This commit adds PID inside lock files that are located under
/run/lock
. More information in #1746Types of changes
Paste Link to the issue
#1746
Checklist
cargo fmt
as mentioned in CODING_GUIDELINEScargo clippy
as mentioned in CODING_GUIDELINESFurther comments