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

consumer: fix send on closed channel when reconnecting #326

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Allenxuxu
Copy link

@Allenxuxu Allenxuxu commented Jun 25, 2021

@mreiferson mreiferson added the bug label Jul 5, 2021
@mreiferson mreiferson changed the title fix send closed channel when consumer reconnecting consumer: fix send on closed channel when reconnecting Jul 5, 2021
@mreiferson
Copy link
Member

Thanks for submitting this @Allenxuxu!

The Consumer exit code is unfortunately extremely convoluted and delicate, and I'm not convinced this fully addresses the issue. I think what needs to happen (at a high-level) is:

  1. Stop()
  2. Prevent reconnections (for both "lookup mode" and "direct connection") by ensuring that we exit lookupdLoop and complete any reconnect goroutines (like you've done here)
  3. Then we need to wait on all connections to close
  4. Stop handlers

Rejects the connection and quickly exits the reconnect loop after the consumer stops.

Signed-off-by: Allenxuxu <120582243@qq.com>
@Allenxuxu
Copy link
Author

@mreiferson I'm sorry it took so long to reply.
I made a few changes:

  1. Check the ’r.stopFlag‘ before add a new connection to ’r.connections‘ , so we can prevent reconnections (for both "lookup mode" and "direct connection")
  2. Quickly exit the reconnect loop after consumer stops.

@mreiferson mreiferson self-requested a review October 24, 2021 20:39
@mreiferson
Copy link
Member

I'm not sure this is sufficient. Stop() doesn't close exitChan, so lookupdLoop() and any single-connection reconnects will still be racing. We need to ensure they all return before exit() gets called.

@Allenxuxu
Copy link
Author

so, maybe we can add stopChan to notify lookupdLoop and single-connection reconnects quickly exit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants