[CORE-29] Migrate from stoppable to BaseApp#Stop. #660
Conversation
CORE-29 Gracefully shutdown daemons
Currently the liquidations daemon and prices daemon do not gracefully shutdown when a validator is stopped. This is an issue in the integration tests because when we shutdown the in-process validators, this can cause the daemon goroutines to panic and the test can fail. However, not panicing is a security risk because it's possible that the validator is running while the daemon (liquidations or prices) is not running. The application assumes that the daemons are always running, and this assumption could lead to subaccounts not getting liquidated / prices not being updated. As a longer term solution, we should do the following:
|
WalkthroughThe changes primarily focus on the removal of the "stoppable" package and associated cleanup functions across multiple files. A new field Changes
TipsChat with CodeRabbit Bot (
|
![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
Files selected for processing (25)
- protocol/app/app.go (7 hunks)
- protocol/app/app_test.go (2 hunks)
- protocol/daemons/pricefeed/client/client_integration_test.go (1 hunks)
- protocol/daemons/pricefeed/client/client_test.go (1 hunks)
- protocol/daemons/server/liquidation_test.go (2 hunks)
- protocol/daemons/server/pricefeed_test.go (4 hunks)
- protocol/daemons/server/server.go (2 hunks)
- protocol/daemons/server/server_test.go (7 hunks)
- protocol/testutil/prices/cli/util.go (2 hunks)
- protocol/x/blocktime/client/cli/query_test.go (2 hunks)
- protocol/x/bridge/client/cli/query_test.go (2 hunks)
- protocol/x/clob/client/cli/cancel_order_cli_test.go (2 hunks)
- protocol/x/clob/client/cli/liquidations_cli_test.go (2 hunks)
- protocol/x/clob/client/cli/place_order_cli_test.go (2 hunks)
- protocol/x/clob/client/cli/query_clob_pair_test.go (2 hunks)
- protocol/x/delaymsg/client/cli/query_test.go (2 hunks)
- protocol/x/epochs/client/cli/query_epoch_info_test.go (2 hunks)
- protocol/x/feetiers/client/cli/query_test.go (2 hunks)
- protocol/x/perpetuals/client/cli/query_perpetual_test.go (2 hunks)
- protocol/x/prices/client/cli/prices_cli_test.go (2 hunks)
- protocol/x/rewards/client/cli/query_params_test.go (2 hunks)
- protocol/x/sending/client/cli/sending_cli_test.go (2 hunks)
- protocol/x/stats/client/cli/query_test.go (2 hunks)
- protocol/x/subaccounts/client/cli/query_subaccount_test.go (2 hunks)
- protocol/x/vest/client/cli/query_vest_entry_test.go (2 hunks)
Files skipped from review due to trivial changes (19)
- protocol/daemons/pricefeed/client/client_integration_test.go
- protocol/daemons/server/liquidation_test.go
- protocol/daemons/server/server.go
- protocol/testutil/prices/cli/util.go
- protocol/x/blocktime/client/cli/query_test.go
- protocol/x/bridge/client/cli/query_test.go
- protocol/x/clob/client/cli/liquidations_cli_test.go
- protocol/x/clob/client/cli/place_order_cli_test.go
- protocol/x/clob/client/cli/query_clob_pair_test.go
- protocol/x/delaymsg/client/cli/query_test.go
- protocol/x/epochs/client/cli/query_epoch_info_test.go
- protocol/x/feetiers/client/cli/query_test.go
- protocol/x/perpetuals/client/cli/query_perpetual_test.go
- protocol/x/prices/client/cli/prices_cli_test.go
- protocol/x/rewards/client/cli/query_params_test.go
- protocol/x/sending/client/cli/sending_cli_test.go
- protocol/x/stats/client/cli/query_test.go
- protocol/x/subaccounts/client/cli/query_subaccount_test.go
- protocol/x/vest/client/cli/query_vest_entry_test.go
Additional comments (Suppressed): 22
protocol/x/clob/client/cli/cancel_order_cli_test.go (2)
6-11: The import of the
stoppablepackage has been removed. Ensure that the functionality provided by this package is no longer needed or has been replaced elsewhere in the codebase.75-79: The cleanup function that was previously stopping all daemon-related services has been removed. If these services are not stopped elsewhere after the tests, this could lead to resource leaks. Please verify that the services are properly stopped after the tests.
protocol/daemons/pricefeed/client/client_test.go (1)
- 302-302: The
uniqueTestIdentifierparameter has been removed from theNewServerfunction call. Ensure that this change does not affect the functionality of the server or any tests that rely on this parameter.protocol/app/app.go (7)
14-19: The import statements remain the same, no changes were made.
94-100: The import statement for the
stoppablepackage has been removed, aligning with the PR's goal of replacing thestoppablepackage withBaseApp#Stop.282-287: A new field
PriceFeedClienthas been added to theAppstruct. Ensure that this new field is properly initialized and used throughout the codebase.546-551: The
appFlags.GrpcAddressargument has been removed from the function call. Ensure that this change does not affect the functionality of the server setup.601-607: The
clientvariable has been replaced withapp.PriceFeedClient. This change aligns with the new field added to theAppstruct.613-618: The
stoppable.RegisterServiceForTestCleanup(appFlags.GrpcAddress, client)line has been removed, which is consistent with the removal of thestoppablepackage.1419-1428: The
Closemethod has been updated to stop thePriceFeedClientandServerif they are notnil. This is a good practice as it ensures that resources are properly released when the application is closed.- return app.BaseApp.Close() + if app.PriceFeedClient != nil { + app.PriceFeedClient.Stop() + } + if app.Server != nil { + app.Server.Stop() + } + return nilprotocol/daemons/server/pricefeed_test.go (4)
23-29: The changes in this hunk are consistent with the changes described in the pull request summary. The
createServerWithMocksfunction now takes atesting.TBparameter and two*mocksparameters. TheWithPriceFeedMarketToExchangePricesmethod is called on the server instance returned bycreateServerWithMocks. This is consistent with the previous code.45-50: The changes in this hunk are consistent with the changes described in the pull request summary. The
createServerWithMocksfunction now takes atesting.TBparameter and two*mocksparameters. TheWithPriceFeedMarketToExchangePricesmethod is not called on the server instance returned bycreateServerWithMocks, which is consistent with the comment above these lines.71-77: The changes in this hunk are consistent with the changes described in the pull request summary. The
createServerWithMocksfunction now takes atesting.TBparameter and two*mocksparameters. TheWithPriceFeedMarketToExchangePricesmethod is called on the server instance returned bycreateServerWithMocks. This is consistent with the previous code.148-154: The changes in this hunk are consistent with the changes described in the pull request summary. The
createServerWithMocksfunction now takes atesting.TBparameter and two*mocksparameters. TheWithPriceFeedMarketToExchangePricesmethod is called on the server instance returned bycreateServerWithMocks. This is consistent with the previous code.protocol/app/app_test.go (2)
1-7: The import statement has been updated to include the
gopkg.in/typ.v4/slicespackage. This package provides utility functions for working with slices in Go. It's used in the new hunk to manipulate theuninitializedFieldsslice.95-108: The test case has been updated to handle the
PriceFeedClientfield. SincePriceFeedClientis hard-coded as disabled inGetDefaultTestAppOptions, it's expected to be uninitialized. The new code checks ifPriceFeedClientis in theuninitializedFieldsslice and removes it if present. This change ensures that the test case doesn't fail due to the expected uninitializedPriceFeedClientfield.- uninitializedFields := getUninitializedStructFields(reflect.ValueOf(*dydxApp)) + uninitializedFields := getUninitializedStructFields(reflect.ValueOf(*dydxApp)) + // Note that the PriceFeedClient is currently hard coded as disabled in GetDefaultTestAppOptions. + // Normally it would be only disabled for non-validating full nodes or for nodes where the + // price feed client is explicitly disabled. + if idx := slices.Index(uninitializedFields, "PriceFeedClient"); idx >= 0 { + slices.Remove(&uninitializedFields, idx) + }protocol/daemons/server/server_test.go (6)
40-43: The
createServerWithMocksfunction now takes an additionaltesting.TBparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.66-72: The
createServerWithMocksfunction now takes an additionaltesting.TBparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.115-121: The
createServerWithMocksfunction now takes an additionaltesting.TBparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.167-170: The
NewServerfunction's signature has been modified to remove theuniqueTestIdentifierparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.183-186: The
NewServerfunction's signature has been modified to remove theuniqueTestIdentifierparameter. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.204-218: The
createServerWithMocksfunction now takes atesting.TBparameter and two*mocksparameters, and it callsmockGrpcServer.On("Stop").Return().Once()andt.Cleanup(server.Stop). This is a significant change from the previous version of the function, which did not have these parameters or calls. Ensure that this change does not introduce any unexpected behavior or side effects.
Changelist
Replace
stoppablewithBaseApp#Stopand remove the global map thatstoppablerelied on.Test Plan
Updated existing tests.
Author/Reviewer Checklist
state-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.