Fix slow shutdown #2113
Fix slow shutdown #2113
Conversation
Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
WalkthroughThe changes in this pull request enhance error handling in the Changes
Assessment against linked issues
Possibly related PRs
Poem
Tip New featuresWalkthrough comment now includes:
Notes:
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? TipsChatThere are 3 ways to chat with CodeRabbit:
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
Documentation and Community
|
Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- cmd/headscale/cli/serve.go (2 hunks)
- hscontrol/app.go (3 hunks)
- hscontrol/notifier/notifier.go (7 hunks)
Additional comments not posted (10)
cmd/headscale/cli/serve.go (2)
4-5: Approved import additions.The new imports for
errorsandnet/httpare necessary for the updated error handling logic and are correctly placed.
28-29: Improved error handling in server shutdown.The updated error handling logic now correctly differentiates between a normal server closure and other errors, preventing the logging of misleading fatal errors during a normal shutdown. This is a significant improvement in the robustness of the application.
hscontrol/notifier/notifier.go (7)
39-39: Approved addition ofclosedfield.The addition of the
closedboolean field to theNotifierstruct is a good practice for managing the lifecycle of the notifier. It helps ensure that no operations are performed on a closed notifier, which can prevent resource leaks and other errors.
47-47: Approved initialization ofclosedfield inNewNotifier.The initialization of the
closedfield tofalsein theNewNotifierfunction is correctly implemented. This ensures that the notifier starts in an active state, ready to handle operations.
85-87: Approved early return inAddNodemethod.The addition of an early return in the
AddNodemethod when the notifier is closed is a good practice. It prevents the addition of new nodes to a closed notifier, maintaining the integrity of the notifier's state and avoiding potential errors.
115-117: Approved early return inRemoveNodemethod.The implementation of an early return in the
RemoveNodemethod when the notifier is closed is correctly done. This prevents modifications to the notifier's state after it has been closed, which is essential for maintaining the robustness of the system.
177-179: Approved check for closed state inNotifyWithIgnore.The addition of a check for the
closedstate in theNotifyWithIgnoremethod is a good practice. It ensures that no updates are sent from a closed notifier, which helps maintain the stability and predictability of the notification system.
197-199: Approved consistency in checking closed state inNotifyByNodeID.The implementation of a check for the
closedstate in theNotifyByNodeIDmethod is consistent with other methods in theNotifierclass. This consistency is crucial for ensuring that the notifier does not perform operations after it has been closed, maintaining the robustness of the system.
236-238: Approved check for closed state insendAll.The implementation of a check for the
closedstate in thesendAllmethod is a good practice. It ensures that updates are not sent to all nodes from a closed notifier, which is crucial for preventing errors that could affect the entire network of nodes.hscontrol/app.go (1)
797-802: Ensure proper resource management during shutdown.The changes made in lines 797-802 address the slow shutdown issue by ensuring that resources are properly managed and released during the shutdown process. The reordering of the shutdown sequence, specifically the closing of the node notifier and waiting for the netmap stream to close, is a critical improvement.
However, it's important to ensure that all resources are accounted for and that there are no potential leaks or deadlocks. The use of
sync.WaitGroupfor managing concurrent operations is appropriate, but thorough testing is necessary to ensure that all go routines are properly synchronized and that theWaitGroupcounter is correctly managed throughout the application lifecycle.Consider adding more detailed logging at each step of the shutdown process to help diagnose issues and ensure that each component is shut down in the correct order and timely manner. This could be particularly useful in production environments where shutdown behaviours might vary due to different loads or configurations.
Additionally, review the entire shutdown sequence to ensure that all network connections, file handles, and other resources are properly closed. This includes verifying that all goroutines are terminated, which can be done by checking for active goroutines before and after the shutdown sequence in a test environment.
Run the following script to verify the proper management of resources during shutdown:
Verification successful
Shutdown sequence is well-managed and logged.
The shutdown sequence in
hscontrol/app.gois comprehensive and includes appropriate use ofsync.WaitGroupand logging mechanisms. The sequence ensures that resources are closed in a specific order, and the logging provides visibility into the process, aiding in diagnosing potential issues. The original review comment's suggestion for detailed logging is already implemented in the codebase.
- The
sync.WaitGroupis used effectively to manage concurrent operations during shutdown.- Logging statements are present throughout the shutdown process, providing detailed insights.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all resources are properly managed during shutdown. # Test: Check for resource leaks or deadlocks during shutdown. Expect: No leaks or deadlocks. # This script assumes the presence of a testing framework that can simulate shutdown scenarios. # Replace `simulate_shutdown` with the actual command or script that triggers the shutdown process. simulate_shutdown # Check system logs or monitoring tools to verify that all resources are released and no errors are reported.Length of output: 104
Script:
#!/bin/bash # Search for the usage of sync.WaitGroup in the codebase to understand its role in the shutdown process. ast-grep --lang go --pattern 'sync.WaitGroup' # Search for functions or methods related to shutdown processes. rg 'shutdown|close|terminate' -A 5 # Check for logging mechanisms related to shutdown processes. rg 'log|trace' -A 5Length of output: 221008
Signed-off-by: Kristoffer Dalby <kristoffer@tailscale.com>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- CHANGELOG.md (1 hunks)
Additional comments not posted (1)
CHANGELOG.md (1)
77-77: Approve the changelog entry with a suggestion.The entry clearly documents the fix for the slow shutdown issue and correctly links to PR #2113. Ensure that the release date
2023-XX-XXfor version0.23.0is updated to the actual release date upon finalising the version.
Turns out we were waiting for connections to close, without telling them to close...
Now:
Fixes #1968
Signed-off-by: Kristoffer Dalby kristoffer@tailscale.com
Summary by CodeRabbit
New Features
Bug Fixes
Documentation