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

try fixing a PartitionConsumer's race condition #1156

Merged
merged 2 commits into from
Mar 13, 2019
Merged

try fixing a PartitionConsumer's race condition #1156

merged 2 commits into from
Mar 13, 2019

Conversation

imjustfly
Copy link
Contributor

@imjustfly imjustfly commented Aug 23, 2018

When closing a PartitionConsumer, a race condition will happen if draining the channel messages . This causes user's consuming routine gets partial messages, which can lead to confusion.

This problem is related to bsm/sarama-cluster#255

@georgeteo
Copy link

This fix may break existing users who expect close to drain the message channel and close to return. It may be possible that these users do not actively drain the message channel, so https://github.com/Shopify/sarama/blob/master/consumer.go#L454 may block, which prevents https://github.com/Shopify/sarama/blob/master/consumer.go#L482, which causes https://github.com/Shopify/sarama/blob/master/consumer.go#L431 to block.

@imjustfly
Copy link
Contributor Author

imjustfly commented Aug 23, 2018

@georgeteo Yes, you are right.

But I still think it's reasonable not draining messages in Close(). I optimized the compatibility.

@varun06
Copy link
Contributor

varun06 commented Mar 4, 2019

@imjustfly What are your view on this PR? is it still relevant?

@imjustfly
Copy link
Contributor Author

@varun06 Yes, I still think it's relevant.

@varun06
Copy link
Contributor

varun06 commented Mar 12, 2019

@sam-obeid you mind taking a quick look?

@varun06
Copy link
Contributor

varun06 commented Mar 12, 2019

@imjustfly I am worried about breaking part mentioned by @georgeteo

@imjustfly
Copy link
Contributor Author

imjustfly commented Mar 13, 2019

@varun06 The 2nd commit in this PR fixed the compatibility breaking issue. You can check if it works.

@varun06
Copy link
Contributor

varun06 commented Mar 13, 2019

Looks good to me 👍 @bai please have a look.

@bai
Copy link
Contributor

bai commented Mar 13, 2019

Thanks!

@harshana-pickme
Copy link

harshana-pickme commented Dec 17, 2020

Looks good to me 👍 @bai please have a look.

@varun06 and @imjustfly please have a look at my comment here . Looks like there is a problem in the behaviour.

@varun06
Copy link
Contributor

varun06 commented Dec 17, 2020

@harshana-pickme will have a look today. Thanks for pointing out.

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.

5 participants