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.

Sign up for GitHub

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

file: Fix race in closing #31

Closed
wants to merge 1 commit into from
Closed

file: Fix race in closing #31

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 16 additions & 5 deletions file.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ type win32File struct {
handle syscall.Handle
wg sync.WaitGroup
closing bool
closingLock sync.RWMutex
readDeadline time.Time
writeDeadline time.Time
}
Expand All @@ -87,9 +88,12 @@ func MakeOpenFile(h syscall.Handle) (io.ReadWriteCloser, error) {

// closeHandle closes the resources associated with a Win32 handle
func (f *win32File) closeHandle() {
if !f.closing {
f.closingLock.Lock()
alreadyClosing := f.closing
f.closing = true
f.closingLock.Unlock()
if !alreadyClosing {
Copy link
Member

@jstarks jstarks Sep 26, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

If alreadyClosing is true, then closeHandle() will no longer block waiting for all IO to complete... Of course this may be better than the current state of things, but I think it might be better to add a wait group or channel to wait for the goroutine that is performing the close.

I would also suggest changing the mutex to just be an atomic.SwapUint32() call in this path, and atomic.LoadUint32() call in isClosing(). We don't need the extra synchronization of the mutex since we are already relying on ordering between the wg.Add(1) call and the read of closing; the atomic makes this ordering contract more explicit and correct without relying on mutual exclusion.

Copy link
Author

Choose a reason for hiding this comment

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

If alreadyClosing is true, that implies that closeHandle() was called from another goroutine. This is the same behavior as today, since the existing code sets f.closing = true before waiting.

Does closeHandle() need to wait when called from multiple goroutines concurrently (does each call to closeHandle() need to wait)?

// cancel all IO and wait for it to complete
f.closing = true
cancelIoEx(f.handle, nil)
f.wg.Wait()
// at this point, no new IO can start
Expand All @@ -98,6 +102,12 @@ func (f *win32File) closeHandle() {
}
}

func (f *win32File) isClosing() bool {
f.closingLock.RLock()
defer f.closingLock.RUnlock()
return f.closing
}

// Close closes a win32File.
func (f *win32File) Close() error {
f.closeHandle()
Expand All @@ -108,7 +118,8 @@ func (f *win32File) Close() error {
// prepareIo prepares for a new IO operation
func (f *win32File) prepareIo() (*ioOperation, error) {
f.wg.Add(1)
if f.closing {
if f.isClosing() {
f.wg.Done()
return nil, ErrFileClosed
}
c := &ioOperation{}
Expand Down Expand Up @@ -142,7 +153,7 @@ func (f *win32File) asyncIo(c *ioOperation, deadline time.Time, bytes uint32, er
var r ioResult
wait := true
timedout := false
if f.closing {
if f.isClosing() {
cancelIoEx(f.handle, &c.o)
} else if !deadline.IsZero() {
now := time.Now()
Expand All @@ -166,7 +177,7 @@ func (f *win32File) asyncIo(c *ioOperation, deadline time.Time, bytes uint32, er
}
err = r.err
if err == syscall.ERROR_OPERATION_ABORTED {
if f.closing {
if f.isClosing() {
err = ErrFileClosed
} else if timedout {
err = ErrTimeout
Expand Down