fix: properly return 503 status when shutdown was initiated #65
Conversation
Signed-off-by: Valery Piashchynski <[email protected]>
WalkthroughThe recent updates enhance the GitHub Actions workflow, linter configurations, and introduce a mechanism for managing graceful shutdowns in the server. Key enhancements include more frequent dependency updates, updated linting tools, and refined linting rules. Additionally, server behavior is improved by adding an Changes
Assessment against linked issues
Poem
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![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
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
go.work.sumis excluded by!**/*.sum
Files selected for processing (9)
- .github/dependabot.yml (1 hunks)
- .github/workflows/linters.yml (1 hunks)
- .golangci.yml (2 hunks)
- health.go (2 hunks)
- jobs.go (2 hunks)
- plugin.go (10 hunks)
- ready.go (2 hunks)
- tests/configs/.rr-status-503.yaml (1 hunks)
- tests/plugin_test.go (2 hunks)
Files skipped from review due to trivial changes (3)
- .github/dependabot.yml
- .golangci.yml
- tests/configs/.rr-status-503.yaml
Additional comments not posted (18)
.github/workflows/linters.yml (1)
19-19: Updated the GitHub Action and linter version to the latest. This is a good practice as it ensures compatibility and access to the latest features and fixes.Also applies to: 21-21
jobs.go (2)
21-21: Introduced ashutdownInitiatedfield to manage shutdown state. Usingatomic.Pointerensures thread safety, which is crucial for concurrent access in a server environment.Also applies to: 24-29
34-37: Properly handles the shutdown scenario by checking theshutdownInitiatedstatus and returning a 503 status code. This aligns with the PR's objectives to improve shutdown behavior.health.go (2)
16-16: AddedshutdownInitiatedto theHealthstruct to manage shutdown states usingatomic.Pointerfor thread safety. Consistent and appropriate use of concurrency mechanisms.Also applies to: 19-24
29-32: Correctly handles the shutdown scenario by checking theshutdownInitiatedstatus and returning a 503 status code. Ensures graceful degradation of service during shutdowns.ready.go (2)
19-19: AddedshutdownInitiatedto theReadystruct to manage shutdown states usingatomic.Pointerfor thread safety. Consistent and appropriate use of concurrency mechanisms.Also applies to: 22-27
32-35: Correctly handles the shutdown scenario by checking theshutdownInitiatedstatus and returning a 503 status code. Ensures readiness checks respond appropriately during shutdowns.plugin.go (9)
8-8: Import ofsync/atomicadded to manage atomic operations, aligning with the new shutdown handling logic.
28-28: Addition of theHasmethod in theConfigurerinterface allows checking for configuration existence, improving robustness.
36-36: Definition of theCheckerinterface is crucial for status checks, ensuring plugins can report their own status.
48-48: Clarification in theReadinessinterface comment helps in understanding the readiness logic related to worker pool status.
57-67: Introduction ofshutdownInitiatedusingatomic.Pointerensures thread-safe handling of shutdown state across various server components.
85-85: Proper initialization ofshutdownInitiatedtofalseat the start of theInitmethod, ensuring correct default state.
96-98: Modification of theServemethod to include shutdown state in handlers. This change is critical for returning a 503 status during shutdowns.
131-132: SettingshutdownInitiatedtotruein theStopmethod ensures that the server correctly signals a shutdown state.
187-189: Utility functiontoPtradded for generic pointer conversion, aiding in clean and reusable code for handling pointers.tests/plugin_test.go (2)
4-4: Import additions to support the testing environment setup, including context handling and HTTP operations.
464-549: New testTestShutdown503added to simulate graceful shutdown and verify that the server returns a 503 status correctly during the shutdown process. This test is crucial for ensuring the new functionality works as intended.
Signed-off-by: Valery Piashchynski <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- tests/configs/.rr-status-503.yaml (1 hunks)
- tests/plugin_test.go (6 hunks)
Files skipped from review as they are similar to previous changes (2)
- tests/configs/.rr-status-503.yaml
- tests/plugin_test.go
Signed-off-by: Valery Piashchynski <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
tests/php_test_files/composer.lockis excluded by!**/*.lock
Files selected for processing (4)
- .gitignore (1 hunks)
- plugin.go (11 hunks)
- tests/configs/.rr-status-503.yaml (1 hunks)
- tests/plugin_test.go (10 hunks)
Files skipped from review due to trivial changes (1)
- .gitignore
Files skipped from review as they are similar to previous changes (1)
- tests/configs/.rr-status-503.yaml
Additional comments not posted (12)
plugin.go (6)
8-8: Added importsync/atomicto handle atomic operations on boolean values for shutdown initiation.
64-65: Introduced ashutdownInitiatedfield to track if a shutdown has been initiated. This is crucial to ensure that the status server can respond appropriately during the shutdown.
85-85: Correctly initializing theshutdownInitiatedatomic pointer tofalseon plugin initialization. This ensures the plugin starts in a non-shutdown state.
96-98: Updated the handlers for/health,/ready, and/jobsto check theshutdownInitiatedstate. This modification is central to the PR's objective of ensuring these endpoints return a 503 status during shutdown.
131-131: Setting theshutdownInitiatedtotruein theStopmethod to signal that a shutdown is in progress. This change is essential for the correct behavior during shutdown.
197-199: Utility functiontoPtradded to simplify the creation of pointers, particularly useful in the context of handling atomic operations withsync/atomic.tests/plugin_test.go (6)
4-4: Import ofcontextadded to support context management in HTTP requests, aligning with best practices for handling request cancellation and timeouts.
36-36: Setup of the test environment using theendurecontainer with a new configuration. This setup is crucial for testing the new functionality in an isolated environment.Also applies to: 39-39, 43-43, 49-49
102-104: Proper cleanup usingt.Cleanupto ensure the HTTP server is stopped after tests, preventing side effects on subsequent tests.
176-178: Repetition of the cleanup pattern in another test function, maintaining consistency across test cases.
248-250: Consistent use of cleanup steps across all test functions, which is a good practice to ensure clean test states.
576-578: Ensuring that the HTTP server is stopped in the cleanup phase of the shutdown test, which is critical for testing the new 503 functionality during shutdown.
Reason for This PR
closes: roadrunner-server/roadrunner#1924
Description of Changes
License Acceptance
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.
PR Checklist
[Author TODO: Meet these criteria.][Reviewer TODO: Verify that these criteria are met. Request changes if not]git commit -s).CHANGELOG.md.Summary by CodeRabbit
New Features
shutdownInitiatedto enhance the health, jobs, plugin, and ready handlers for better management of service shutdowns.TestShutdown503to simulate and verify graceful shutdown scenarios with HTTP status checks.Chores
golangci/golangci-lint-actionto versionv6.0.1andgolangci-linttov1.59..gitignoreto include composer lock files.Configuration Changes
.golangci.yml.rr-status-503.yaml.