-
Notifications
You must be signed in to change notification settings - Fork 221
fixes exec queue add and pop #7591
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
Conversation
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.
Pull request overview
This PR fixes issues with the execution queue's Add and Pop operations, addressing concurrency problems and improving error handling in the async execution system.
Key Changes
- Fixed the Pop() method condition from
> 1to>= 1to correctly handle single-item queues - Updated AddOrReplace notification logic to only notify when the first item is added to an empty queue
- Added ValidateQueueIntegrity() method for periodic queue health checks
- Improved error handling with proper error propagation and retry logic with exponential backoff
- Enhanced error handling in baseSync.go using errors.Is() instead of direct comparison
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| process/asyncExecution/queue/blocksQueue.go | Fixed Pop() condition bug, restructured notification logic, added ValidateQueueIntegrity() method, and enhanced logging |
| process/asyncExecution/headersExecutor.go | Added retry mechanism with max attempts and exponential backoff, periodic validation ticker, nil checks for headers/bodies, and fixed error propagation |
| process/sync/baseSync.go | Updated error comparison to use errors.Is() and ensured deferred error handling works correctly |
| process/asyncExecution/queue/errors.go | Removed unused ErrMissingHeaderNonce and added ErrQueueIntegrityViolation |
| process/asyncExecution/errors.go | Added new error types for nil handlers and execution results |
| process/interface.go | Added ValidateQueueIntegrity() to BlocksQueue interface |
| process/asyncExecution/interface.go | Added ValidateQueueIntegrity() to BlocksQueue interface |
| testscommon/processMocks/blocksQueueMock.go | Added ValidateQueueIntegrityCalled field and method to mock |
| process/asyncExecution/queue/blocksQueue_test.go | Added comprehensive test coverage for ValidateQueueIntegrity() and concurrent operations |
| process/asyncExecution/headersExecutor_test.go | Fixed test expectations to properly validate error propagation |
Comments suppressed due to low confidence (1)
process/asyncExecution/headersExecutor.go:143
- This select statement with a default case creates a busy-wait loop when the validation ticker doesn't fire. The default case will be executed continuously, causing high CPU usage even when the queue is empty and Pop() is blocking. Consider removing the default case or adding a small sleep in the default branch to avoid busy-waiting. The ticker and Pop() blocking should provide sufficient responsiveness without the default case.
select {
case <-ctx.Done():
return
case <-validationTicker.C:
// Periodic queue validation
err := he.blocksQueue.ValidateQueueIntegrity()
if err != nil {
log.Error("headersExecutor.start: queue integrity validation failed", "err", err)
}
default:
he.mutPaused.RLock()
isPaused := he.isPaused
he.mutPaused.RUnlock()
if isPaused {
time.Sleep(timeToSleep)
continue
}
// blocking operation
headerBodyPair, ok := he.blocksQueue.Pop()
if !ok {
log.Debug("headersExecutor.start: not ok fetching from queue")
// close event
return
}
if check.IfNil(headerBodyPair.Header) || check.IfNil(headerBodyPair.Body) {
log.Debug("headersExecutor.start: popped nil header or body, continuing...")
continue
}
err := he.process(headerBodyPair)
if err != nil {
he.handleProcessError(ctx, headerBodyPair)
}
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if err != nil { | ||
| log.Warn("headersExecutor.process process block failed", | ||
| "nonce", pair.Header.GetNonce(), | ||
| "hash", pair.Header.GetPrevHash(), |
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.
| "hash", pair.Header.GetPrevHash(), | |
| "prevHash", pair.Header.GetPrevHash(), |
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.
done
| // Validate input parameters | ||
| if check.IfNil(pair.Header) { |
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.
don't think its really needed, there are already some nil checks before process call
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.
removed
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.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The base branch was changed.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/supernova-async-exec #7591 +/- ##
==========================================================
Coverage 77.68% 77.68%
==========================================================
Files 876 876
Lines 121033 121120 +87
==========================================================
+ Hits 94029 94097 +68
- Misses 20796 20816 +20
+ Partials 6208 6207 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Fix trigger epoch start on boot
fix consensus revert
fix pending mini blocks computation for epoch start data on v3 metablock
Reasoning behind the pull request
Proposed changes
Testing procedure
Pre-requisites
Based on the Contributing Guidelines the PR author and the reviewers must check the following requirements are met:
featbranch created?featbranch merging, do all satellite projects have a proper tag insidego.mod?