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 deadlock when connection closed #376

Merged
merged 7 commits into from
Oct 9, 2020

Conversation

jiazhai
Copy link
Member

@jiazhai jiazhai commented Oct 1, 2020

Fixes #366

Motivation

In current code of pulsar/internal/connection.go we have 2 channels, closeCh and incomingRequestsCh. when the connection closes, the current mis-use of these 2 channels may have a deadlock.
PR #366 has detailed steps to reproduce and the root cause analysis .
This PR tries to fix the deadlock.

Modifications

  • make the close logic independent, not in the same loop of normal events handling.
  • when the connection closed, handle the existing requests in the channel and return an error to avoid deadlock.

Verifying this change

passed the tests in #366
current ut passed

@jiazhai jiazhai self-assigned this Oct 1, 2020
@jiazhai jiazhai changed the title Fix deadlock when connection closed [WIP]Fix deadlock when connection closed Oct 1, 2020
Signed-off-by: xiaolong.ran <rxl@apache.org>
@wolfstudy wolfstudy changed the title [WIP]Fix deadlock when connection closed Fix deadlock when connection closed Oct 9, 2020
@wolfstudy wolfstudy added this to the 0.3.0 milestone Oct 9, 2020
Signed-off-by: xiaolong.ran <rxl@apache.org>
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.

Cool, LGTM+1

@jiazhai
Copy link
Member Author

jiazhai commented Oct 9, 2020

Thanks @wolfstudy for the help

Signed-off-by: xiaolong.ran <rxl@apache.org>
Signed-off-by: xiaolong.ran <rxl@apache.org>
Signed-off-by: xiaolong.ran <rxl@apache.org>
Signed-off-by: xiaolong.ran <rxl@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants