pkg/lock: Close the file descriptor on unsuccessful locking #3616
Conversation
|
Can one of the admins verify this patch? |
|
doesn't this conflict with #3615? |
|
Yes it does but created separate PR as these two PRs fix two different issues. |
shobhit85
force-pushed
the
fix-leaking-fds
branch
from
d772b35
to
7f6d8e3
Compare
|
ok to test |
|
I agree that |
| return nil, err | ||
| } | ||
| // Check if the file is a regular file | ||
| if lockType == RegFile && !(stat.Mode&syscall.S_IFMT == syscall.S_IFREG) { | ||
| l.Close() |
There was a problem hiding this comment.
As per comment, I'd only keep these two chunks here in NewLock and adjust the docstring accordingly.
|
/cc @euank as he had opinions on the other similar-but-unrelated PR on the lock module. |
|
@lucab Closing fd in other ancillary functions seems necessary as caller of these functions doesn't have any handle to the fd if just err is returned. So those fds are leaked. I'll update |
|
@shobhit85 you are right, I got confused by those other functions acting on a |
|
@lucab Updated the NewLock docstring. If looks good I'll go ahead and squash the commits. |
|
@shobhit85 yes please. |
shobhit85
force-pushed
the
fix-leaking-fds
branch
from
3595e3e
to
774b3be
Compare
NewLock() opens a new file descriptor to lock on but if file is already locked, Try*Lock() methods return error without closing the corresponding file descriptor.
shobhit85
force-pushed
the
fix-leaking-fds
branch
from
774b3be
to
0addfd4
Compare
|
Done with rebase and squash. cc @lucab |
|
@lucab Can this be merged? |
|
@shobhit85 I just dismissed my stale review and re-triggered CI which was left in a confused state. |
|
@lucab That semaphoreci is still failing. Not sure why. |
NewLock() opens a new file descriptor to lock on but if file
is already locked, Try*Lock() methods return error without closing
the corresponding file descriptor.