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 heartbeat goroutine leak #149

Closed
wants to merge 5 commits into from
Closed

Fix heartbeat goroutine leak #149

wants to merge 5 commits into from

Conversation

SPCDTS
Copy link

@SPCDTS SPCDTS commented Jul 7, 2023

fix #142

  1. In connection.StopAllConsuming(), call connection.stopHeartbeat() to avoid heartbeat goroutine leak when all queue-finish-channels closed .
  2. move connection.heartbeatStop = nil from connection.stopHeartbeat() to heartbeat() to avoid dead lock while reach heartbeat error limits.
  3. revise cleaner_test to match modification.
  4. rename StopAllConsuming to Close for semantic reason.

@wellle
Copy link
Member

wellle commented Jan 29, 2024

Hey, thank you for this PR. I started to have a look at it recently and I believe I found an issue:

  1. In general I don't think it's safe to do heartbeatStop = nil on the caller side, as it's not protected by the lock then.
  2. More specifically consider the case where heartbeat() runs into HeartbeatErrorLimit case: In your implementation you set heartbeatStop = nil before calling Close(), so Close would actually bail out early, not stopping consumers nor the heartbeat.

I took the same approach though and opened #162.

I'm closing this PR. Thank you for your contribution though 👍

@wellle wellle closed this Jan 29, 2024
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.

heartbeat is not stopped
2 participants