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

KAFKA-2311: Consumer's ensureNotClosed method not thread safe #1637

Closed
wants to merge 1 commit into from

Conversation

@tbrooks8
Copy link
Contributor

tbrooks8 commented Jul 19, 2016

Here is the patch on github @ijuma.

Acquiring the consumer lock (the single thread access controls) requires that the consumer be open. I changed the closed variable to be volatile so that another thread's writes will visible to the reading thread.

Additionally, there was an additional check if the consumer was closed after the lock was acquired. This check is no longer necessary.

This is my original work and I license it to the project under the project's open source license.

@tbrooks8

This comment has been minimized.

Copy link
Contributor Author

tbrooks8 commented Jul 20, 2016

I see that the SaslPlaintextConsumerTest failed on the build. This is a little surprising to me as I'm not clear why my change would have broken something. And the tests are passing for me.

But I'll take a harder look tomorrow and see if I can figure out why that test failed.

@ijuma

This comment has been minimized.

Copy link
Contributor

ijuma commented Jul 20, 2016

It's probably unrelated to your PR.

@tbrooks8

This comment has been minimized.

Copy link
Contributor Author

tbrooks8 commented Jul 22, 2016

Yes. After taking a look at these tests more in depth, I do see a clear reason why my change would have caused this failure.

@hachikuji

This comment has been minimized.

Copy link
Contributor

hachikuji commented Sep 13, 2016

LGTM

@asfgit asfgit closed this in 2a660f1 Sep 13, 2016
ijuma added a commit to ijuma/kafka that referenced this pull request Sep 15, 2016
Here is the patch on github ijuma.

Acquiring the consumer lock (the single thread access controls) requires that the consumer be open. I changed the closed variable to be volatile so that another thread's writes will visible to the reading thread.

Additionally, there was an additional check if the consumer was closed after the lock was acquired. This check is no longer necessary.

This is my original work and I license it to the project under the project's open source license.

Author: Tim Brooks <tim@uncontended.net>

Reviewers: Jason Gustafson <jason@confluent.io>

Closes apache#1637 from tbrooks8/KAFKA-2311
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.