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

[Issue 213] Fix data race when accessing partition producer state #215

Merged
merged 1 commit into from
May 18, 2020

Conversation

cornelk
Copy link
Contributor

@cornelk cornelk commented Apr 12, 2020

Fixes #213

Motivation

Panic due to a race condition.

Modifications

  • access to state of partitionProducer is using atomic operations now to be thread-safe
  • partitionConsumer will probably need a similar fix as well

Verifying this change

  • Make sure that the change passes the CI checks.

  • go test -race -count 100 -v -run 'TestDLQ$' . does not panic anymore

This change is already covered by existing tests, such as TestDLQ.

Does this pull request potentially affect one of the following parts:

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: (no)
  • The default values of configurations: (no)
  • The wire protocol: (no)

Documentation

  • Does this pull request introduce a new feature? (no)

@sijie sijie requested review from wolfstudy and merlimat and removed request for wolfstudy April 13, 2020 01:50
@jiazhai jiazhai requested a review from wolfstudy April 15, 2020 02:34
@jiazhai
Copy link
Member

jiazhai commented Apr 15, 2020

Thanks for the great help @cornelk.
@cornelk @wolfstudy Do you think it is possible to add some test like:
"go test -race -count 100 -v -run 'TestDLQ$' ",
into the github action CI process?

@wolfstudy
Copy link
Member

Thanks for the great help @cornelk.
@cornelk @wolfstudy Do you think it is possible to add some test like:
"go test -race -count 100 -v -run 'TestDLQ$' ",
into the github action CI process?

Good idea, will add this to ci

Copy link
Member

@wolfstudy wolfstudy left a comment

Choose a reason for hiding this comment

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

LGTM +1

@wolfstudy
Copy link
Member

ping @merlimat

@cornelk can you help to check whether this problem also exists on the consumer side?

@wolfstudy wolfstudy merged commit bc647c6 into apache:master May 18, 2020
@wolfstudy wolfstudy added this to the 0.1.1 milestone May 18, 2020
@cornelk cornelk deleted the fix_producer_close_panic branch May 18, 2020 06:36
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.

Race access of partitionProducer.state
3 participants