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

[Issue859] fix panic when waiting on left requests to drain upon connection close #860

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

zzzming
Copy link
Contributor

@zzzming zzzming commented Oct 12, 2022

Fixes #859

Master Issue: #859

Motivation

The problem is basically WaitGroup does not allow Wait() and Add() to run concurrently. (Add() itself can run concurrently). This is clearly explained in the Go WaitGroup source code at https://github.com/golang/go/blob/master/src/sync/waitgroup.go#L30

// Add adds delta, which may be negative, to the WaitGroup counter.
// If the counter becomes zero, all goroutines blocked on Wait are released.
// If the counter goes negative, Add panics.
//
// Note that calls with a positive delta that occur when the counter is zero
// must happen before a Wait. Calls with a negative delta, or calls with a
// positive delta that start when the counter is greater than zero, may happen
// at any time.
// Typically this means the calls to Add should execute before the statement
// creating the goroutine or other event to be waited for.

Modifications

Just to remove the WaitGroup, incomingRequestsWG. Because the use of WaitGroup is incorrect and the code logic has changed thus no longer is WG required.

Here are justificaitons

  1. Add() and Wait() cannot be called concurrently in Go.
  2. The code was introduced by PR Fix write to closed channel panic() in internal/connection during connection close #539 There was another call TriggerClose() has been merged into Close() since. The condition has changed since.
  3. failLeftRequestsWhenClose() has changed that it uses a nil signal to determine as the last signal in the channel
  4. We should reply on the state check in SendRequest and SendRequestWait

Verifying this change

  • Make sure that the change passes the CI checks.

Does this pull request potentially affect one of the following parts:

If yes was chosen, please highlight the changes

  • Dependencies (does it add or upgrade a dependency): (no)
  • The public API: (no)
  • The schema: ( no )
  • The default values of configurations: ( no)
  • The wire protocol: (no)

Documentation

  • Does this pull request introduce a new feature? (/ no)
  • If yes, how is the feature documented? (not applicable )
  • If a feature is not applicable for documentation, explain why?
  • If a feature is not documented yet in this PR, please create a followup issue for adding the documentation

@zzzming zzzming changed the title [Issue859] fix close connection panic when waiting on left requests to drain [Issue859] fix panic when waiting on left requests to drain upon connection close Oct 12, 2022
Copy link
Contributor

@Gleiphir2769 Gleiphir2769 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch!

incomingRequestsWG is useless and just remove it would be fine.

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.

[BUG] Panics in SendRequestNoWait are triggered  when reconnecting consumer to broker
2 participants