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

core/identity: add enabler #5084

Merged
merged 5 commits into from
Apr 26, 2024
Merged

Conversation

calebdoxsey
Copy link
Contributor

@calebdoxsey calebdoxsey commented Apr 23, 2024

Summary

Add a new Enabler to simplify enabling and disabling components dynamically.

Checklist

  • reference any related issues
  • updated docs
  • updated unit tests
  • updated UPGRADING.md
  • add appropriate tag (improvement / bug / etc)
  • ready for review

@coveralls
Copy link

coveralls commented Apr 23, 2024

Coverage Status

coverage: 57.328% (+0.1%) from 57.207%
when pulling a8c7134 on cdoxsey/identity-manager-enabled
into 05e077f on main.

Copy link
Contributor

@kenjenkins kenjenkins left a comment

Choose a reason for hiding this comment

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

Thanks, I think something like this could make sense. Added a couple comments.

internal/disabler/disabler.go Outdated Show resolved Hide resolved
internal/disabler/disabler.go Outdated Show resolved Hide resolved
@calebdoxsey
Copy link
Contributor Author

I renamed it to enabler and re-worked the logic to use a context.CancelCauseFunc for signaling from Enable/Disable to Run.

@calebdoxsey calebdoxsey changed the title core/identity: add disabler core/identity: add enabler Apr 25, 2024
@calebdoxsey calebdoxsey marked this pull request as ready for review April 25, 2024 15:52
@calebdoxsey calebdoxsey requested a review from a team as a code owner April 25, 2024 15:52
@calebdoxsey calebdoxsey requested a review from wasaga April 25, 2024 15:52
@calebdoxsey calebdoxsey added the enhancement New feature or request label Apr 25, 2024
Copy link
Contributor

@kenjenkins kenjenkins left a comment

Choose a reason for hiding this comment

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

@wasaga, would you mind taking a look at this as well? I don't really trust myself when reasoning about concurrency.

Comment on lines 60 to 66
for {
err := d.runOnce(ctx)
if errors.Is(err, errCauseEnabler) {
continue
}
return err
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I find this loop a little confusing, but I think I'm slowly understanding. Let me see if I can summarize the behavior:

  • This loop will keep going until the underlying context is canceled for some reason unrelated to the enabled/disabled state.
  • If currently enabled, runOnce() will synchronously call RunEnabled(), blocking until that function returns.
  • If currently disabled, runOnce() will still block, waiting for a transition to the enabled state.

I think the name "runOnce" confused me a little (because it might not run anything), but I don't have any naming alternatives to offer.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I changed the loop to avoid the continue.

When a ctx is involved like this I try to have the loop separate from the implementation so that the scope of the new ctx doesn't override the original one (a subtle bug that has bit me several times before), and also so we can use normal returns to exit early instead of continues.

I renamed the method to make it more clear and added some comments.

Basically we get the current enabled state, create a new cancelable context and call the cancel function in Enable and Disable.

In the case when we're enabled, we call RunEnabled with the new context which will be canceled by a call to Disable. We then check the underlying cause. If it's due to us, we return that error, which is checked in the loop. But any other error (or nil) we want to exit to preserve the original behavior of Run.

In the case when we're disabled, we just wait for the context to be canceled, which can only happen if the parent context is canceled or we cancel it with a call to Enable. context.Cause should surface that error, and the loop will continue if the cause was us, or return if the parent context was canceled.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, I like the name runOrWaitForEnabled.

@calebdoxsey calebdoxsey merged commit 99a5dbd into main Apr 26, 2024
16 checks passed
@calebdoxsey calebdoxsey deleted the cdoxsey/identity-manager-enabled branch April 26, 2024 21:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants