🐛 Fixes Respect Keyboard interrupt kantra#776#1133
Conversation
See: konveyor/kantra#776 Signed-off-by: John Matthews <jwmatthews@gmail.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ffered channel Replace buffered abConfigChan with unbuffered channel and wrap the send in a select with providerInitCtx.Done() so sender goroutines exit cleanly on cancellation. Move waitGroup.Done() from receiver to sender to ensure it always runs. More idiomatic Go approach to the ctrl-c deadlock fix. Signed-off-by: John Matthews <jwmatthews@gmail.com> Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
shawn-hurley
left a comment
shawn-hurley
left a comment
There was a problem hiding this comment.
This is really good and helpful. This has been a nagging issue that I think should be solved.
Thank you!
| select { | ||
| case <-c: | ||
| a.log.V(3).Info("started all non builtin providers") | ||
| case <-a.ctx.Done(): |
There was a problem hiding this comment.
We should add timeout to the providerInitCtx, and then update this to use the providerInitCtx.
It should be built off to a.ctx so that if a.ctx is done, it will provide providerInitCtx, and providerInitCtx timeout is a part of the context. Then we can remove the if/else and just have a single select on c or providerInitCtx.Done().
Let me know if that makese sense or not?
| Total: len(a.providers), | ||
| }) | ||
| abConfigChan <- additionalBuiltins | ||
| select { |
There was a problem hiding this comment.
This is fixing a channel drain on when context is done.
But the fix I don't think is achieving the same result, we are now marking the wait group finished when additional builtins are passed to the channel, not when the the go routine that adds the additonal builtins to the slice is completed.
If the channel has a buffer of 1. there will still be a lag between updating the slice and the WaitGroup.Wait() finishing, and this I think could cause a race condition.
the waitGroup.Done() I think needs to be called original location, we just need to add on cancel to all call waitGroup.Done().
For draining the waitGroup, there could be at most 1 other object in the buffer.
But, I think that the fix below, around adding context to if noTimeout, actually fully solves the issue that you are trying to solve, without having to worry about this.
| rule.scope = scopes | ||
| rule.carrier = carrier | ||
| r.ruleProcessing <- rule | ||
| select { |
There was a problem hiding this comment.
a couple of things:
- I think I would be hard-pressed to agree on using the goTo here.
- Why add a buffer to the return channel?
To add support for canceling w/ the working group, we should follow a similar pattern in analyzer.go where we have the wait in a go routine, and when we finish, we close a sentinel channel, with a select that selects on ctx.Done() or the close of the channel.
|
|
||
| // We need some wrapper that can deal with out of tree providers, this will be a call, that will mock it out, but go against in tree. | ||
| func GetProviderClient(config provider.Config, log logr.Logger, progress *progress.Progress) (provider.InternalProviderClient, error) { | ||
| func GetProviderClient(ctx context.Context, config provider.Config, log logr.Logger, progress *progress.Progress) (provider.InternalProviderClient, error) { |
There was a problem hiding this comment.
I don't know if we need to do this or if we want to do this,
But this will break consumers of the lib package.
Can we add a GetProviderClientWithContext function and update this function to call it with the context set to background?
What are your thoughts?
|
This pull request has been automatically marked as stale because it has not had any activity for 60 days. |
2 participants
See: konveyor/kantra#776
analyzer-lsp fixes:
Related to konveyor/kantra#777
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com