fix: ensure node and services are watcing for the same shutdown signal from the context cancel - #1406

• node/full.go (2 hunks)
• node/light.go (2 hunks)

node/full.go (1)

• 95-107: The changes to the newFullNode function correctly implement the creation of a context with cancel and defer the cancel function to ensure it is called in case of an error. This aligns with the PR objectives to ensure that nodes and services respond to the same shutdown signal.

node/light.go (4)

• 49-61: The changes to the newLightNode function signature and the addition of context handling with a deferred cancel function are consistent with the PR objectives. This ensures that the services receive the shutdown signal when the node shuts down. The use of named return values for error handling is idiomatic in Go.

• 55-60: The implementation of the context creation with context.WithCancel(ctx) and the deferred cancel function is correct. It ensures that the cancel function is called if an error occurs, which is a good practice for resource cleanup.

• 81-81: The error handling in the newLightNode function correctly uses fmt.Errorf with the %w verb to wrap errors, which allows for error unwrapping and is considered a best practice in Go.

• 81-86: The OnStop method correctly calls the cancel function to initiate the shutdown process. However, it's important to ensure that all errors from stopping services are handled properly. The use of multierr.Append is a good practice to collect all errors.

• block/manager.go (3 hunks)
• node/full_client.go (2 hunks)
• node/full_node_integration_test.go (4 hunks)
• node/node_test.go (1 hunks)
• state/txindex/indexer_service.go (2 hunks)
• test/context/context.go (1 hunks)

block/manager.go (3)

• 386-393: The addition of context cancellation checks at the start of the trySyncNextBlock function is a good practice to ensure that the function can exit early if the context is canceled. This is in line with the PR's objective to improve shutdown signal handling.

• 523-528: The context cancellation check added to the processNextDABlock function is consistent with the changes made in trySyncNextBlock. This ensures that the function respects the context's cancellation signal and can return early if necessary.

• 634-639: The context cancellation check added to the publishBlock function is consistent with the other changes and follows the same pattern of checking for context cancellation at the start of the function. This ensures that the function does not proceed if the context has been canceled.

node/full_client.go (2)

• 112-118: The change to use the provided context for unsubscribing from the event bus in the BroadcastTxCommit method aligns with the PR's objective to improve context management. This ensures that the unsubscribe operation is aware of the same context that was used for subscription, which can help prevent potential leaks or unexpected behavior during shutdown.

• 885-891: The update to the resubscribe method to use the context from the client's node (c.node.ctx) instead of context.Background() is consistent with the PR's goal of ensuring that nodes and services are watching for the same shutdown signal. This change should help prevent issues where services might not receive a shutdown signal, as they will now use the same context as the node.

node/full_node_integration_test.go (3)

• 71-76: The addition of context creation and cancellation before the newFullNode function call is consistent with the PR's objective to ensure that nodes and services respond to the same shutdown signal. This change should help prevent the issue where services were not shutting down properly during tests.

• 488-497: The retry mechanism added for ensuring that blocks are DA included is a good addition for robustness in tests. It's important to verify that the retry logic is correctly implemented and that the error messages are clear for debugging purposes.

• 71-76: The changes made to the context handling in the test functions align with the PR's objective and improve the consistency of shutdown signal handling. It's important to ensure that the context is cancelled at the right time to avoid resource leaks, especially in the context of testing where multiple tests may run in sequence or parallel.

node/node_test.go (1)

• 20-24: The removal of the deferred node.Cancel() call in cleanUpNode function changes the cleanup behavior. Ensure that the node's resources are properly released and that there are no side effects due to this change, such as resource leaks or incomplete shutdowns.

state/txindex/indexer_service.go (2)

• 47-60: The change to use is.ctx in the SubscribeUnbuffered calls aligns with the PR's objective to ensure consistent context management across nodes and services. This should allow for proper shutdown handling when the context is canceled.

• 130-135: The update to use is.ctx in the UnsubscribeAll call within the OnStop method is consistent with the PR's goal of unified context management, which should help in proper resource cleanup during shutdown.

test/context/context.go (1)

• 10-31: The implementation of WithDebugCancelFunc looks correct and should provide valuable debugging information when the context is canceled. It captures the stack trace of the last 5 calls before the cancel function is invoked and prints them, which can help in tracing the source of premature or unexpected cancellations.