-
Notifications
You must be signed in to change notification settings - Fork 262
Fix fialed to watch context canceled error when streaming logs
#2681
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,6 +16,7 @@ package pipelinerun | |
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "sync" | ||
| "time" | ||
|
|
||
|
|
@@ -68,6 +69,11 @@ func (t *Tracker) Monitor(allowed []string) <-chan []taskrunpkg.Run { | |
|
|
||
| genericInformer, _ := factory.ForResource(*gvr) | ||
| informer := genericInformer.Informer() | ||
|
|
||
| // Set a custom watch error handler that ignores context.Canceled errors | ||
| // to prevent "Failed to watch" log messages when the informer is stopped intentionally | ||
| _ = informer.SetWatchErrorHandlerWithContext(watchErrorHandler) | ||
|
|
||
| mu := &sync.Mutex{} | ||
| stopC := make(chan struct{}) | ||
| trC := make(chan []taskrunpkg.Run) | ||
|
|
@@ -154,7 +160,15 @@ func pipelinerunOpts(name string) func(opts *metav1.ListOptions) { | |
| return func(opts *metav1.ListOptions) { | ||
| opts.FieldSelector = fields.OneTermEqualSelector("metadata.name", name).String() | ||
| } | ||
| } | ||
|
|
||
| // watchErrorHandler is a custom watch error handler that filters out context.Canceled errors | ||
| // to prevent "Failed to watch" log messages when the informer is stopped intentionally. | ||
| // Other errors are passed to the default handler. | ||
| func watchErrorHandler(ctx context.Context, r *cache.Reflector, err error) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Code Duplication (Optional) This Options:
Given the simplicity, the current approach is perfectly reasonable. This is more of an FYI than a required change. |
||
| if !errors.Is(err, context.Canceled) { | ||
| cache.DefaultWatchErrorHandler(ctx, r, err) | ||
| } | ||
| } | ||
|
|
||
| // handles changes to pipelinerun and pushes the Run information to the | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -15,6 +15,8 @@ | |
| package pipelinerun | ||
|
|
||
| import ( | ||
| "context" | ||
| "errors" | ||
| "fmt" | ||
| "testing" | ||
| "time" | ||
|
|
@@ -239,3 +241,28 @@ func startPipelineRun(t *testing.T, data pipelinetest.Data, prStatus ...v1.Pipel | |
| Dynamic: dynamic, | ||
| } | ||
| } | ||
|
|
||
| func TestTracker_watchErrorHandler(t *testing.T) { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ✅ Excellent test coverage! Love that you're testing both direct The approach of passing |
||
| tests := []struct { | ||
| name string | ||
| err error | ||
| }{ | ||
| { | ||
| name: "context.Canceled should be filtered", | ||
| err: context.Canceled, | ||
| }, | ||
| { | ||
| name: "wrapped context.Canceled should be filtered", | ||
| err: errors.Join(errors.New("watch failed"), context.Canceled), | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(_ *testing.T) { | ||
| // Call watchErrorHandler with context.Canceled errors | ||
| // These should be filtered (not passed to DefaultWatchErrorHandler) | ||
| // so passing nil reflector is safe | ||
| watchErrorHandler(context.Background(), nil, tt.err) | ||
| }) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused return value
The error return from
SetWatchErrorHandlerWithContextis ignored here. This is probably fine since it's a configuration step during initialization, but if you wanted to be more defensive, you could log any errors:That said, the current approach is acceptable for simplicity.