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

Fix hypothetical consumer deadlock, attempt 2 #484

Merged
merged 1 commit into from
Jul 27, 2015
Merged

Conversation

eapache
Copy link
Contributor

@eapache eapache commented Jul 15, 2015

Fixes bug #475

Should be correct in all cases, unlike #483.

@Shopify/kafka

@eapache eapache force-pushed the fix-consumer-deadlock branch from fc66764 to 4a7c496 Compare July 15, 2015 17:42
@eapache
Copy link
Contributor Author

eapache commented Jul 15, 2015

As I should have realized when the original ticket confused willem, the over-use of dying was the root problem. That value doesn't need synchronization, so just put it as a normal member.

@wvanbergen
Copy link
Contributor

:shipit: - this looks a lot easier to understand as well 👍

eapache added a commit that referenced this pull request Jul 27, 2015
Fix hypothetical consumer deadlock, attempt 2
@eapache eapache merged commit 527a3d3 into master Jul 27, 2015
@eapache eapache deleted the fix-consumer-deadlock branch July 27, 2015 13:43
@horkhe
Copy link
Contributor

horkhe commented Jul 27, 2015

@eapache @wvanbergen this change introduce a race over closing the dying channel. So PartitionConsumer.Close() may fail with panic: close of closed channel. And that is the reason why a build of #492 failed after rebase to master.

@eapache
Copy link
Contributor Author

eapache commented Jul 27, 2015

In my opinion that test is broken: it is user error to call Close or AsyncClose on a consumer after it has already closed, which it must have done because the Messages channel returned !ok.

@horkhe
Copy link
Contributor

horkhe commented Jul 27, 2015

Consider this:

select {
case msg, ok := <-pc.Messages():
    if !ok {
        break // The consumer is closed
    }
    pc.Close() // Panic!!! because the channel was closed right after we checked the channel
}

@horkhe
Copy link
Contributor

horkhe commented Jul 27, 2015

In other words since there is no atomic operation checkChannelAndClose the Close() operation is not reliable anymore.

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.

3 participants