Propagate ctx cancellation to IndexLogs #811
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
WalkthroughThe changes update logging behavior in two contract broadcaster constructors by removing the inclusion of the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant retry
participant Context
Caller->>retry: retry(ctx, logger, sleep, address, fn)
loop Retry Loop
retry->>fn: Call fn()
alt fn returns nil
retry-->>Caller: return nil
else fn returns retryable error
retry->>Context: Check ctx.Done
alt ctx cancelled
retry-->>Caller: return ctx.Err()
else not cancelled
retry->>Context: Wait for sleep or ctx.Done
alt ctx cancelled during sleep
retry-->>Caller: return ctx.Err()
else sleep elapsed
retry->>retry: Next retry iteration
end
end
end
end
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Add context cancellation support to IndexLogs retry mechanism in log handler
📍Where to StartStart with the Macroscope summarized 18c42af. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
pkg/indexer/common/log_handler.go (1)
178-197: Excellent implementation of context cancellation handling.The changes properly handle context cancellation in two key places:
- Before executing the function (lines 178-181)
- During the sleep between retries (lines 187-192)
This implementation follows Go's best practices for context handling by:
- Using
selectstatements to check for cancellation- Returning
ctx.Err()when canceled- Maintaining the original behavior when not canceled
Consider adding a brief comment to the function documentation explaining that it respects context cancellation:
func retry( ctx context.Context, logger *zap.Logger, sleep time.Duration, address string, fn func() re.RetryableError, + // retry executes the given function repeatedly until it succeeds or returns a non-retryable error. + // It respects context cancellation and will return ctx.Err() if the context is canceled. ) error {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
pkg/indexer/app_chain/contracts/group_message.go(0 hunks)pkg/indexer/app_chain/contracts/identity_update.go(0 hunks)pkg/indexer/common/log_handler.go(2 hunks)
💤 Files with no reviewable changes (2)
- pkg/indexer/app_chain/contracts/group_message.go
- pkg/indexer/app_chain/contracts/identity_update.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
pkg/indexer/common/log_handler.go (2)
pkg/errors/errors.go (1)
RetryableError(5-8)pkg/metrics/indexer.go (1)
EmitIndexerRetryableStorageError(99-102)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd-cli)
- GitHub Check: Push Docker Images to GitHub Packages (xmtpd)
- GitHub Check: Test (Node)
- GitHub Check: Upgrade Tests
🔇 Additional comments (2)
pkg/indexer/common/log_handler.go (2)
147-149: Good update to propagate context in retry function call.The change correctly forwards the context to the retry function, enabling proper cancellation propagation. This ensures that when the parent context is canceled, the retry operation can respond appropriately.
170-171: Well-implemented context parameter addition.Adding the context parameter as the first argument follows Go conventions. This change properly enables context cancellation support in the retry function.
| select { | ||
| case <-ctx.Done(): | ||
| return ctx.Err() | ||
| case <-time.After(sleep): |
There was a problem hiding this comment.
@coderabbitai can you check whether time.after works in a loop? Can you show us the relevant docs.
There was a problem hiding this comment.
🧩 Analysis chain
Based on the script execution results, I need to search for detailed information about time.After behavior in loops.
@mkysel Based on my research, using time.After() in a loop can be problematic and is generally discouraged in Go.
According to the Go blog (Timers and cancellation section in Timer-based Patterns), time.After() creates a new timer each time it's called, and "The timer created by time.After is not recovered by the garbage collector until the timer fires." This means if the loop iterates frequently, it can accumulate many timers in memory, potentially causing a memory leak.
From the Go documentation, a better approach is to create a timer once and reuse it:
func retry(
ctx context.Context,
logger *zap.Logger,
sleep time.Duration,
address string,
fn func() re.RetryableError,
) error {
timer := time.NewTimer(0) // Create timer once
if !timer.Stop() { // Stop initial firing
<-timer.C
}
for {
select {
case <-ctx.Done():
if !timer.Stop() {
// Drain the channel if timer fired
select {
case <-timer.C:
default:
}
}
return ctx.Err()
default:
if err := fn(); err != nil {
logger.Error("error storing log", zap.Error(err))
if err.ShouldRetry() {
metrics.EmitIndexerRetryableStorageError(address)
timer.Reset(sleep) // Reuse the timer
select {
case <-ctx.Done():
if !timer.Stop() {
<-timer.C
}
return ctx.Err()
case <-timer.C:
continue
}
}
return err
}
return nil
}
}
}This pattern avoids creating a new timer on each iteration, reducing memory pressure and potential leaks.
(｡･ω･｡) thoughtfully taps paw on notepad
🏁 Script executed:
#!/bin/bash
# Look for Go documentation about time.After in the codebase
grep -r "time.After" --include="*.go" . | head -20
# Check the imports to see if standard Go time package is used
head -20 pkg/indexer/common/log_handler.go
# Let's examine the context of the loop more carefully
cat pkg/indexer/common/log_handler.go | grep -A 30 "func retry"
Length of output: 1415
🌐 Web query:
Does time.After in Go cause memory leaks when used in loops?
💡 Result:
In Go, the time.After function returns a channel that will receive the current time after at least the specified duration has elapsed. When used repeatedly in loops without proper handling, it can indeed cause resource issues akin to memory leaks, but it's more about goroutine and timer resource consumption than classic memory leaks.
Here’s why:
- Each call to
time.After(d)creates a new timer internally. - If the timer is not read from or properly drained by receiving from the channel, the timer remains active until it fires.
- If you create many timers in a tight loop and do not receive from their channels (or the timer hasn’t fired yet), these timers accumulate and consume memory and resources.
- This buildup can lead to increased memory usage and eventually degrade performance.
Better approach in loops
Instead of using time.After repeatedly in loops, consider:
- Using a single
time.Timerand resetting it when needed (time.NewTimerandReset). - Using
time.Tickerif you want repeated intervals. - Directly using
time.Sleepif blocking behavior is acceptable.
Example problem with time.After in loop
for i := 0; i < 1000000; i++ {
<-time.After(time.Second)
// do something
}This can cause increased memory usage as each timer is created and not immediately garbage collected.
Alternative using time.NewTimer:
timer := time.NewTimer(time.Second)
defer timer.Stop()
for i := 0; i < 1000000; i++ {
<-timer.C
timer.Reset(time.Second)
// do something
}This approach reuses a single timer and avoids the accumulation problem.
Summary
time.Aftercan cause resource buildup if used in tight loops without consumption of the timer events.- This is often mistaken for memory leaks but is about timers and goroutine resources.
- Use
time.NewTimer+Resetortime.Tickerfor looping timed operations to avoid this issue.
If you want, I can provide more detailed code examples or further explanation!
@mkysel Yes, time.After() works functionally in a loop, but it's considered a potential memory leak risk in Go.
According to the Go documentation and community best practices, each call to time.After() creates a new timer that remains in memory until it fires. When used in a loop, this can cause resource buildup as new timers are continuously created.
From the Go Timer documentation:
The Timer type represents a single event. When the Timer expires, the current time will be sent on C, unless the Timer was created by AfterFunc.
The current code pattern:
for {
// ...
select {
// ...
case <-time.After(sleep):
continue
}
}Creates a new timer on each iteration where a retry is needed, which is inefficient.
A better approach is to use a reusable timer:
func retry(ctx context.Context, logger *zap.Logger, sleep time.Duration, address string, fn func() re.RetryableError) error {
// Create a timer once
timer := time.NewTimer(0)
if !timer.Stop() {
<-timer.C
}
for {
// Function execution and error handling
if err := fn(); err != nil && err.ShouldRetry() {
// Reset the timer for next use
timer.Reset(sleep)
select {
case <-ctx.Done():
if !timer.Stop() {
<-timer.C
}
return ctx.Err()
case <-timer.C:
continue
}
}
// Other code...
}
}This pattern creates only one timer and reuses it throughout the function's lifecycle.
🧬 Code Graph Analysis Results
Relevant snippet from pkg/indexer/common/log_handler.go (lines 110-143):
func retry(
ctx context.Context,
logger *zap.Logger,
sleep time.Duration,
address string,
fn func() re.RetryableError,
) error {
for {
select {
case <-ctx.Done():
return ctx.Err()
default:
if err := fn(); err != nil {
logger.Error("error storing log", zap.Error(err))
if err.ShouldRetry() {
metrics.EmitIndexerRetryableStorageError(address)
select {
case <-ctx.Done():
return ctx.Err()
case <-time.After(sleep):
continue
}
}
return err
}
return nil
}
}
}This function retry runs a loop that calls a function fn returning a RetryableError. If the error indicates it should retry (ShouldRetry() returns true), it waits for a specified sleep duration using time.After(sleep) before retrying. The loop also listens for context cancellation (ctx.Done()).
Relevant snippet from pkg/errors/errors.go (lines 5-8):
RetryableError interface {
error
ShouldRetry() bool
}This interface defines the error type used in the retry function, which includes a method ShouldRetry() to indicate if the error is retryable.
Summary of time.After usage in the retry loop:
time.After(sleep)returns a channel that blocks until the specified duration elapses.- The
selectstatement waits on either the context cancellation or the timer channel. - This pattern is used inside a
forloop to implement a retry with delay. - This confirms that
time.Afterworks correctly in a loop and can be used to pause execution for a duration before retrying.
Additional context:
- The retry loop is designed to keep retrying the operation until it succeeds, the context is canceled, or a non-retryable error occurs.
- The use of
time.Afterinside theselectstatement is idiomatic Go for implementing timed waits that can be interrupted by context cancellation.
[no other relevant snippets]
✏️ Learnings added
Learnt from: mkysel
PR: xmtp/xmtpd#811
File: pkg/indexer/common/log_handler.go:190-190
Timestamp: 2025-05-19T15:09:12.359Z
Learning: Using `time.After()` in loops can cause memory leaks in Go because each call creates a new timer that remains in memory until it fires. The recommended pattern is to create a timer once with `time.NewTimer()` outside the loop and reuse it with `timer.Reset()` inside the loop.
Learnt from: mkysel
PR: xmtp/xmtpd#811
File: pkg/indexer/common/log_handler.go:190-190
Timestamp: 2025-05-19T15:09:12.359Z
Learning: Using `time.After()` in loops can cause memory leaks in Go because each call creates a new timer. The recommended pattern is to create a single timer with `time.NewTimer()` outside the loop and reuse it with `timer.Reset()` inside the loop.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
IndexLogs.retry().Summary by CodeRabbit
Bug Fixes
Refactor