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: Call wg.Add in main goroutine to avoid race conditons. #3433

Merged
merged 1 commit into from Nov 7, 2019

Conversation

@beautytiger
Copy link
Contributor

beautytiger commented Nov 7, 2019

1. Why is this pull request needed and what does it do?

If the goroutine call wg.Add by itself, what if the main goroutine run to the end before goroutine start runing? May there are chances.

2. Which issues (if any) are related?

3. Which documentation changes (if any) need to be made?

4. Does this introduce a backward incompatible change or deprecation?

No

Signed-off-by: Guangming Wang <guangming.wang@daocloud.io>
@corbot corbot bot requested a review from chrisohaver Nov 7, 2019
@corbot

This comment has been minimized.

Copy link

corbot bot commented Nov 7, 2019

Thank you for your contribution. I've just checked the OWNERS files to find a suitable reviewer. This search was successful and I've asked chrisohaver (via plugin/transfer/OWNERS) for a review.
Note this is not an exclusive request. Anyone is free to provide a review of this pull request.

If you have questions or suggestions for this bot, please file an issue against the miekg/dreck repository.

The bot understands the commands that are listed here.

@miekg

This comment has been minimized.

Copy link
Member

miekg commented Nov 7, 2019

how does this relate to the docs?

    40  // Add adds delta, which may be negative, to the WaitGroup counter.
    41  // If the counter becomes zero, all goroutines blocked on Wait are released.
    42  // If the counter goes negative, Add panics.
    43  //
    44  // Note that calls with a positive delta that occur when the counter is zero
    45  // must happen before a Wait. Calls with a negative delta, or calls with a
    46  // positive delta that start when the counter is greater than zero, may happen
    47  // at any time.
    48  // Typically this means the calls to Add should execute before the statement
    49  // creating the goroutine or other event to be waited for.
    50  // If a WaitGroup is reused to wait for several independent sets of events,
    51  // new Add calls must happen after all previous Wait calls have returned.
    52  // See the WaitGroup example.
@beautytiger

This comment has been minimized.

Copy link
Contributor Author

beautytiger commented Nov 7, 2019

I think it would be this:

// Typically this means the calls to Add should execute before the statement
// creating the goroutine or other event to be waited for.

call the wg.Add before creating the goroutine.

@chrisohaver

This comment has been minimized.

Copy link
Member

chrisohaver commented Nov 7, 2019

The same change to file/xfr.go (from where this code was originally copied from) was merged in #3302.

Copy link
Member

chrisohaver left a comment

LGTM, especially since the same change was merged to the original code from where this was copied. Referencing #3302 would have made this more immediately obvious.

@miekg miekg merged commit 113783e into coredns:master Nov 7, 2019
5 checks passed
5 checks passed
DCO DCO
Details
ci/circleci: kubernetes-tests Your tests passed on CircleCI!
Details
codecov/project 56.47% (target 50%)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
stickler-ci No lint errors found.
gonzalop added a commit to gonzalop/coredns that referenced this pull request Nov 18, 2019
Signed-off-by: Guangming Wang <guangming.wang@daocloud.io>
gonzalop added a commit to gonzalop/coredns that referenced this pull request Nov 18, 2019
Signed-off-by: Guangming Wang <guangming.wang@daocloud.io>
Signed-off-by: Gonzalo Paniagua Javier <gonzalo.mono@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.