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

Jump to bottom

notify: fix data race and send on close chan in receiveCh #566

Merged
merged 2 commits into from
May 13, 2020
Merged

notify: fix data race and send on close chan in receiveCh #566

merged 2 commits into from
May 13, 2020

Conversation

amyangfei
Copy link
Contributor

@amyangfei amyangfei commented May 13, 2020 •
edited
Loading

What problem does this PR solve?

The receiverCh is not closed from the sender routine.

➜  GO111MODULE=on go test -check.f TestContinusStop
panic: send on closed channel

goroutine 3445 [running]:
github.com/pingcap/ticdc/pkg/notify.signalNonBlocking(0xc000b510e0)
        /home/tidb/code/ticdc/pkg/notify/notify.go:29 +0x35
github.com/pingcap/ticdc/pkg/notify.(*Notifier).NewReceiver.func1(0xc000b3b720, 0xc000b510e0)
        /home/tidb/code/ticdc/pkg/notify/notify.go:55 +0x5c
created by github.com/pingcap/ticdc/pkg/notify.(*Notifier).NewReceiver
        /home/tidb/code/ticdc/pkg/notify/notify.go:53 +0x30b
exit status 2
FAIL    github.com/pingcap/ticdc/pkg/notify     0.126s

This error also happens in integration test: https://internal.pingcap.net/idc-jenkins/blue/rest/organizations/jenkins/pipelines/cdc_ghpr_kafka_integration_test/runs/216/nodes/113/steps/541/log/?start=0

What is changed and how it works?

Add a closeCh to be used as the close barrier, only close receiveCh in the ticker loop.

Check List

Tests

  • Unit test
  • Integration test

Release note

  • No release note

@amyangfei
Copy link
Contributor Author

/run-integration-tests

@amyangfei
Copy link
Contributor Author

/run-integration-tests

@amyangfei amyangfei mentioned this pull request May 13, 2020
Merged
@amyangfei
Copy link
Contributor Author

/run-kafka-tests

@amyangfei amyangfei added the status/ptal Could you please take a look? label May 13, 2020
crazycs520
crazycs520 reviewed May 13, 2020
View reviewed changes
Copy link
Contributor

@crazycs520 crazycs520 left a comment

Choose a reason for hiding this comment

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

LGTM

zier-one
zier-one approved these changes May 13, 2020
View reviewed changes
Copy link
Contributor

@zier-one zier-one left a comment

Choose a reason for hiding this comment

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

LGTM

@zier-one zier-one merged commit 8c515b7 into pingcap:master May 13, 2020
@zier-one zier-one added LGT2 and removed LGT1 labels May 13, 2020
@amyangfei amyangfei deleted the fix-notify-panic branch May 13, 2020 07:09
5kbpers pushed a commit to 5kbpers/ticdc that referenced this pull request Aug 24, 2020
@amyangfei
notify: fix data race and send on close chan in receiveCh (pingcap#566)
4c4bf53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@crazycs520 crazycs520 crazycs520 left review comments

@zier-one zier-one zier-one approved these changes

Assignees
No one assigned
Labels
status/ptal Could you please take a look?
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@amyangfei @crazycs520 @zier-one