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

fix go race bug #2262

Merged
merged 2 commits into from
Sep 21, 2016
Merged

fix go race bug #2262

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0d5eca9
fix go race bug
WIZARD-CXY Aug 10, 2016
a570bf6
Update gated_writer.go
WIZARD-CXY Aug 14, 2016
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
35 changes: 24 additions & 11 deletions command/agent/gated_writer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,15 +29,28 @@ func (w *GatedWriter) Flush() {
}

func (w *GatedWriter) Write(p []byte) (n int, err error) {
w.lock.RLock()
defer w.lock.RUnlock()

if w.flush {
return w.Writer.Write(p)
}

p2 := make([]byte, len(p))
copy(p2, p)
w.buf = append(w.buf, p2)
return len(p), nil
// Once we flush we no longer synchronize writers since there's
Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @WIZARD-CXY thanks for the PR!

I think the read lock was intended to not synchronize writers after the flush was done (since there's no way to set flush to false again, and after startup it will basically be going through that path forever). You are right there's a race here, though. I think this fix would have the intended behavior:

func (w *GatedWriter) Write(p []byte) (n int, err error) {
        // Once we flush we no longer synchronize writers since there's
        // no use of the internal buffer. This is the happy path.
        w.lock.RLock()
        if w.flush {
                w.lock.RUnlock()
                return w.Writer.Write(p)
        }
        w.lock.RUnlock()

        // Now take the write lock.
        w.lock.Lock()
        defer w.lock.Unlock()

        // Things could have changed between the locking operations, so we
        // have to check one more time.
        if w.flush {
                return w.Writer.Write(p)
        }

        // Buffer up the written data.
        p2 := make([]byte, len(p))
        copy(p2, p)
        w.buf = append(w.buf, p2)
        return len(p), nil
}

Copy link
Contributor Author

@WIZARD-CXY WIZARD-CXY Aug 14, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Perfect! I will change according to your suggestion

// no use of the internal buffer. This is the happy path.
w.lock.RLock()
if w.flush {
w.lock.RUnlock()
return w.Writer.Write(p)
}
w.lock.RUnlock()

// Now take the write lock.
w.lock.Lock()
defer w.lock.Unlock()

// Things could have changed between the locking operations, so we
// have to check one more time.
if w.flush {
return w.Writer.Write(p)
}

// Buffer up the written data.
p2 := make([]byte, len(p))
copy(p2, p)
w.buf = append(w.buf, p2)
return len(p), nil
}