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

executor: make sure hashjoin's goroutine exit before Close return #8338

Merged
merged 5 commits into from
Nov 20, 2018
Merged

executor: make sure hashjoin's goroutine exit before Close return #8338

merged 5 commits into from
Nov 20, 2018

Conversation

lysu
Copy link
Contributor

@lysu lysu commented Nov 16, 2018

What problem does this PR solve?

fixes #8337

What is changed and how it works?

also wait fetchInnerRows goroutine exit when found finished be setted.

Check List

Tests

  • Old test

Code changes

  • Close logic

Side effects

  • N/A

Related changes

  • Need to cherry-pick to the release branch

This change is Reviewable

@lysu lysu added the sig/execution SIG execution label Nov 16, 2018
@lysu
Copy link
Contributor Author

lysu commented Nov 16, 2018

/run-all-tests

Copy link
Contributor

@eurekaka eurekaka left a comment

Choose a reason for hiding this comment

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

Have you checked other parallel operators?

executor/join.go Outdated Show resolved Hide resolved
Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

LGTM

@XuHuaiyu XuHuaiyu added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 16, 2018
@alivxxx alivxxx mentioned this pull request Nov 19, 2018
@lysu
Copy link
Contributor Author

lysu commented Nov 19, 2018

@eurekaka I only check hashjoin and this new groutine is introduce in those two month.

@lysu
Copy link
Contributor Author

lysu commented Nov 19, 2018

/run-all-tests

executor/join.go Outdated Show resolved Hide resolved
@lysu
Copy link
Contributor Author

lysu commented Nov 19, 2018

@zz-jason PTAL

&

/run-all-tests

executor/join.go Outdated
e.fetchInnerDone.Add(1)
go util.WithRecovery(
func() { e.fetchInnerRows(ctx, innerResultCh, doneCh) },
func(r interface{}) { e.finishFetchInnerRows(innerResultCh) },
Copy link
Member

@zz-jason zz-jason Nov 19, 2018

Choose a reason for hiding this comment

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

Why only finishFetchInnerRows() when the fetchInnerRows() goroutine is paniced?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not~ it alway call finishFetchInnerRows but with r ==nil

Copy link
Member

Choose a reason for hiding this comment

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

😂 I was mislead by the r interface{} parameter in the function call.

Copy link
Member

Choose a reason for hiding this comment

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

can we move the cleaning code into that function, and only do the recover work in this function?

Copy link
Member

@zz-jason zz-jason left a comment

Choose a reason for hiding this comment

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

LGTM

@zz-jason zz-jason added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Nov 20, 2018
@eurekaka eurekaka merged commit c5bd5b6 into pingcap:master Nov 20, 2018
@lysu lysu deleted the hash-join-close-race-fix branch November 20, 2018 11:31
lysu added a commit that referenced this pull request Nov 20, 2018
crazycs520 pushed a commit to crazycs520/tidb that referenced this pull request Nov 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/execution SIG execution status/LGT2 Indicates that a PR has LGTM 2. type/bugfix This PR fixes a bug.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hash-join executor close returned but forked goroutine still running
4 participants