Add an explicit Close function #21
Conversation
virtuald
commented
Aug 7, 2018
virtuald
commented
- Useful for long running processes that don't want to depend on the GC for cleanup
There was a problem hiding this comment.
Thank you for this contribution! I've found a few small things I think need to be tweaked before we merge this.
I like clean/linear git history that really reflects the changes being made. So when going through to address the comments, please amend your current commit instead of creating additional ones.
flock.go
Outdated
| f.m.Lock() | ||
| defer f.m.Unlock() | ||
|
|
||
| f.unlockInternal() |
There was a problem hiding this comment.
Please handle the returned error here.
There was a problem hiding this comment.
How would you like to deal with it? It seems like the same type of thing as line 55.
There was a problem hiding this comment.
In this case the most reasonable is likely to annotate the error and return it upward.
There was a problem hiding this comment.
Hmm. Well, would that be right? What happens if someone closes after Unlock()? Does it make sense to make it so that Close() just fails if Unlock() hasn't been called yet?
There was a problem hiding this comment.
I've been doing some thinking about this, and from my perspective I think I see two possible ways to go about solving it. I'm coming at it from the perspective of wanting to know when unsafe data access may have been happening. If I hit an error when trying to release the exclusive lock I have on a file, that may indicate I've been silently corrupting data and may want to manually validate consistency.
So I think there're two ways we can make sure that information isn't lost:
- The first would be to simply return the error from
f.unlockInternal()if one occurs. I don't recall whether calling.Unlock()and then.Close()would result inf.unlockInternal()returning an error for on.Close(). If it does... - The second option is to disallow closing when the file is still locked. This would require consumers to be more thoughtful of how they consume the package, and to do the orchestration in the right order.
@virtuald Which path do you feel better aligns with the problem you're solving?
There was a problem hiding this comment.
I think disallowing closing when the file is still locked is unintuitive and would lead to difficult to diagnose issues (particularly if one did an unchecked defer lock.Close() which seems to be a common if ill-advised pattern).
According to fcntl(2):
If a process closes any file descriptor referring to a file, then all of the process's locks on that file are released, regardless of the file descriptor(s) on which the locks were obtained.
According to MSDN for LockFile:
If a process terminates with a portion of a file locked or closes a file that has outstanding locks, the locks are unlocked by the operating system. However, the time it takes for the operating system to unlock these locks depends upon available system resources. Therefore, it is recommended that your process explicitly unlock all files it has locked when it terminates. If this is not done, access to these files may be denied if the operating system has not yet unlocked them.
From both of these perspectives, I think it's a valid thing to just ignore the unlock error, as Close will also unlock the file. If any error should be returned, I would say the error returned from Close() should be returned as that's the semantic thing that's occurring.
flock_windows.go
Outdated
| } | ||
|
|
||
| // the internal mutex must be held for reading+writing when calling this function | ||
| func (f *Flock) unlockInternal() error { | ||
|
|
There was a problem hiding this comment.
Please remove the unnecessary whitespace after the method header.
flock_unix.go
Outdated
| } | ||
|
|
||
| // the internal mutex must be held for reading+writing when calling this function | ||
| func (f *Flock) unlockInternal() error { | ||
|
|
There was a problem hiding this comment.
Please remove this unnecessary whitespace after the method header.
flock.go
Outdated
| // Close will call Unlock() and close the underlying file descriptor. Use of this | ||
| // function is not strictly necessary as the file descriptor will be cleaned up | ||
| // by the garbage collector, but some long lived applications may not want to | ||
| // depend on that |
There was a problem hiding this comment.
Slight nit, but because the result of the comment has punctuation can you add a period on the end to align with the rest of the package comments?
virtuald
force-pushed
the
explicit-close
branch
4 times, most recently
from
7c77bdd
to
9823d7f
Compare
|
Changed to return any error from Close. |
|
Oh. Heh. I hadn't noticed that unlock also closes the file descriptor. Hm. I suppose this isn't really necessary then -- though, I feel like I would really want to ensure that the descriptor gets closed, regardless of whether there's an error that occurs when unlocking? |
flock_unix.go
Outdated
| @@ -85,7 +85,11 @@ func (f *Flock) lock(locked *bool, flag int) error { | |||
| func (f *Flock) Unlock() error { | |||
There was a problem hiding this comment.
Can you please move this function out to flock.go and remove it from flock_unix.go and flock_windows.go? With having the unexported unlockInternal method, we don't need the duplicated Unlock anymore.
flock.go
Outdated
| // function is not strictly necessary as the file descriptor will be cleaned up | ||
| // by the garbage collector, but some long lived applications may not want to | ||
| // depend on that. | ||
| func (f *Flock) Close() error { |
There was a problem hiding this comment.
I think we should keep this function, but maybe just have it invoke return f.Unlock()
|
@virtuald might you have a spare moment to look at this one more time? |
|
I can take a look this weekend. |
virtuald
force-pushed
the
explicit-close
branch
from
9823d7f
to
37b0e02
Compare
|
Since Unlock already does what I wanted, I took your suggestion and just added a close function that calls Unlock. |
Add an explicit Close function Signed-off-by: Tim Heckman <t@heckman.io>
