ci: fix panic in indexer #1402
Merged
ci: fix panic in indexer #1402
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
WalkthroughThe recent updates to the Go codebase focus on enhancing context management and thread safety. Contexts with cancellation capabilities have been introduced to certain functions, allowing for better control over goroutines. Additionally, mutex locks have been added to various methods to ensure that concurrent access to shared resources is properly synchronized, preventing race conditions. Changes
Poem
TipsChat with CodeRabbit Bot (
|
MSevey
added
T:bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (1)
- node/full_client_test.go (1 hunks)
Additional comments: 2
node/full_client_test.go (2)
171-179: The changes in the
TestGenesisChunkedfunction correctly implement the addition of a cancellable context to prevent panics during test execution. The context is created and deferred cancellation is set up properly. This should enhance the test's reliability by allowing for graceful shutdowns and cleanup.176-176: The update to the
newFullNodefunction call to use the cancellable context (ctx) instead of a background context is in line with the PR's objective to prevent test panics by allowing for context cancellation.
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1402 +/- ##
==========================================
+ Coverage 51.00% 51.09% +0.08%
==========================================
Files 46 46
Lines 6417 6422 +5
==========================================
+ Hits 3273 3281 +8
+ Misses 2804 2802 -2
+ Partials 340 339 -1 ☔ View full report in Codecov by Sentry. |
MSevey
force-pushed
the
sevey/fix-indexer-panic
branch
from
24a0979
to
e860f3e
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (2)
- node/full_client_test.go (2 hunks)
- state/txindex/indexer_service.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- node/full_client_test.go
Additional comments: 1
state/txindex/indexer_service.go (1)
- 74-99: The changes in the loop correctly implement the non-blocking select statement to handle context cancellation signals. This allows the loop to exit gracefully when a cancellation signal is received, which aligns with the PR's objective to improve stability and context management. The error handling within the loop also seems to follow the
terminateOnErrorpattern, which is consistent with the rest of the service's design.
S1nus
previously approved these changes
Dec 8, 2023
Manav-Aggarwal
previously approved these changes
Dec 8, 2023
Manav-Aggarwal
force-pushed
the
sevey/fix-indexer-panic
branch
from
6bf0054
to
0eb8555
Compare
Manav-Aggarwal
approved these changes
Dec 8, 2023
S1nus
approved these changes
Dec 8, 2023
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
Manav-Aggarwal
force-pushed
the
sevey/fix-indexer-panic
branch
from
a55f36c
to
d429d97
Compare
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Configuration used: CodeRabbit UI
Files selected for processing (3)
- mempool/clist_mempool.go (2 hunks)
- node/full_client_test.go (2 hunks)
- state/txindex/indexer_service.go (1 hunks)
Files skipped from review as they are similar to previous changes (2)
- mempool/clist_mempool.go
- state/txindex/indexer_service.go
Additional comments: 1
node/full_client_test.go (1)
- 171-179: The changes to
TestGenesisChunkedcorrectly implement the use of a cancellable context, which is passed to thenewFullNodefunction. This aligns with the PR objectives to improve context management and cleanup in tests.
github-merge-queue bot
pushed a commit
that referenced
this pull request
Dec 15, 2023
…l from the context cancel (#1406) <!-- Please read and fill out this form before submitting your PR. Please make sure you have reviewed our contributors guide before submitting your first PR. --> ## Overview <!-- Please provide an explanation of the PR, including the appropriate context, background, goal, and rationale. If there is an issue with this information, please provide a tl;dr and link the issue. --> The nodes were creating a new context with cancel after passing the original context through to the services. This meant that when the node shutdown and cancelled its context, its services did not receive a shutdown signal until the original caller cancelled its context. This was leading to panics in testing about writing to a logger test file that was already closed. This change was verified by reverting the changes in #1402 and testing in a loop 100 times with no panics. **EDIT 1** Since no good deed goes unpunished 🙃 some timeouts surfaced. This was due to `context.Background()` being used for subscribe and unsubscribe events. Under the hood those events are watching for `ctx.Done()` events, so when `context.Background()` is passed in, they can hang indefinitely if the other signals aren't triggered. The test that was most prone to this timeout was run in a loop 1000 times to verify the issue was fixed. Additionally, added some more checks for `ctx.Done()` for faster shutdowns. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced node shutdown process with context cancellation for improved service termination. - **Refactor** - Updated error handling and context management in node creation functions. - Improved context usage in block synchronization and event subscription methods. - **Tests** - Adjusted full node integration tests to reflect new context management. - **Chores** - Removed redundant node cancellation in test cleanup function. - **Documentation** - No visible changes to end-users. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
3 fixes:
Summary by CodeRabbit
Refactor
Tests