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 config duplication detection and throttle goroutine lifecycle. #2728

Conversation

timoreimann
Copy link
Contributor

This change fixes two problems:

  • Configurations were compared against the persisted configuration, not the latest received; the two may deviate when throttling is active. We introduce a local configuration cache and compare against that.
  • The variable managing throttling goroutines was only scoped to the local method, effectively disabling goroutine cleanup and producing a leak. We properly hook up the goroutines to live as long as the server does.

We also refactor the throttling logic into a dedicated struct. While not completely separating the concerns of the server from the throttler (we still need to reference a number of server methods/values inside the throttler), it's a step forward.

Additional changes:

  • Pass the stop signal from listenProviders through to the throttler eventually, avoiding the need to close the server for a proper shutdown.
  • Exit throttler if ring buffer is closed. (Could happen before stop signal is processed.)
  • Test: Return published configs through channel to rule out race conditions.
  • Test: Invoke stop channel function earlier to trigger goroutine shutdown.

This change fixes two problems:

- Configurations were compared against the persisted configuration, not
  the latest received; the two may deviate when throttling is active. We
  introduce a local configuration cache and compare against that.
- The variable managing throttling goroutines was only scoped to the
  local method, effectively disabling goroutine cleanup and producing a
  leak. We properly hook up the goroutines to live as long as the server
  does.

We also refactor the throttling logic into a dedicated struct. While not
completely separating the concerns of the server from the throttler (we
still need to reference a number of server methods/values inside the
throttler), it's a step forward.

Additional changes:

- Pass the stop signal from listenProviders through to the throttler
  eventually, avoiding the need to close the server for a proper shutdown.
- Exit throttler if ring buffer is closed. (Could happen before stop
  signal is processed.)
- Test: Return published configs through channel to rule out race
  conditions.
- Test: Invoke stop channel function earlier to trigger goroutine
  shutdown.
@timoreimann timoreimann added status/2-needs-review kind/bug/confirmed a confirmed bug (reproducible). labels Jan 19, 2018
@traefiker traefiker added this to the 1.5 milestone Jan 19, 2018
@ldez ldez added contributor/waiting-for-corrections kind/bug/fix a bug fix and removed kind/bug/confirmed a confirmed bug (reproducible). labels Jan 19, 2018
@timoreimann
Copy link
Contributor Author

Closing this one in favor of smaller, more digestable PRs. The first one is already out, the others will follow (they are not that critical).

@timoreimann timoreimann deleted the fix-config-dup-detection-and-throttler-channel-management branch May 8, 2018 11:53
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.

None yet

3 participants