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.

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

merged 2 commits into from
Sep 21, 2016

Conversation

WIZARD-CXY
Copy link
Contributor

I'm using consul as part of my program, when I enable -race in the go test, I found this small bug, since w.buf = append(w.buf, p2) can be a write operation.

@@ -29,8 +29,8 @@ func (w *GatedWriter) Flush() {
}

func (w *GatedWriter) Write(p []byte) (n int, err error) {
w.lock.RLock()
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

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

@WIZARD-CXY
Copy link
Contributor Author

@slackpad PTAL

@WIZARD-CXY
Copy link
Contributor Author

@slackpad PTAL thx

@slackpad slackpad merged commit b0b05f4 into hashicorp:master Sep 21, 2016
@slackpad
Copy link
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants