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

log context cancellation reasons #33

Merged
merged 6 commits into from
May 17, 2024
Merged

log context cancellation reasons #33

merged 6 commits into from
May 17, 2024

Conversation

apesternikov
Copy link
Contributor

It is hard to debug a test failure caused by finished context. Possible reasons are:

  • timeout
  • signal
  • stdin closed
  • error watching k8s events
    Better logging of the reason for context cancellation should help in this case.

Comment on lines +349 to +351
ctx, timeoutCancel := context.WithTimeoutCause(context.Background(), *timeout, ErrTimedOut)
defer timeoutCancel()
ctx, cancel := context.WithCancelCause(ctx)

Choose a reason for hiding this comment

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

The context.WithTimeoutCause and context.WithCancelCause functions do not exist in the standard Go context package. This could lead to compilation errors if these are not custom implementations. If these are custom implementations, it's recommended to ensure they are well-documented and tested, considering the critical nature of context handling in concurrent Go applications. If they are intended to be the standard context.WithTimeout and context.WithCancel, replacing them with the correct functions from the standard library is advised to avoid confusion and potential runtime issues.

@@ -378,13 +382,13 @@ func main() {
if err != nil {
log.Print(err)
}
cancel()
cancel(fmt.Errorf("terminate due to kubernetes listening failure: %w", err))

Choose a reason for hiding this comment

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

Cancelling the context with an error message constructed using fmt.Errorf inside a deferred function that runs in a goroutine can lead to a race condition where the main function may exit before the deferred function has a chance to cancel the context. This can result in the error message not being logged or acted upon as intended. A more reliable approach would be to ensure that all goroutines have completed their execution and any necessary context cancellation has been performed before allowing the main function to exit. This might involve using synchronization primitives like sync.WaitGroup to wait for goroutines to finish their execution.

Comment on lines 365 to +367
_, _, err := reader.ReadRune()
if err != nil && err == io.EOF {
cancel()
cancel(ErrStdinClosed)

Choose a reason for hiding this comment

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

The error handling logic in the goroutine that reads from os.Stdin checks if the error is io.EOF using a redundant condition. The condition err != nil && err == io.EOF can be simplified to just err == io.EOF since err == io.EOF implicitly covers the case where err is not nil. Simplifying this condition will improve the readability of the code without changing its functionality.

@apesternikov apesternikov marked this pull request as ready for review May 17, 2024 20:13
@apesternikov apesternikov merged commit 526acc1 into main May 17, 2024
2 checks passed
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.

None yet

2 participants