[Bug]: simapp and Network do not invoke Close()

[lcwik]

Is there an existing issue for this?

• I have searched the existing issues

What happened?

I was looking for a shutdown hook that I can use in a Cosmos application. I found the Close() method that was added but noticed that it is not invoked during Network#Cleanup() and it also doesn't look like simapp calls Close() either.

Furthermore it looks like simapp does use testutil/testnet/nodes.go which utilize CometBFT Service#Stop().

Network and simapp should invoke the Close() shutdown hook.

[Optionally] Cosmos SDK should integrate into CometBFTs shutdown hook (OnStop() override OR Quit() channel) instead of re-inventing the wheel.

Cosmos SDK Version

0.47.4

How to reproduce?

Tests written using simapp / Network do not invoke the shutdown hook.

[julienrbrt]

Server is calling close on interrupt signal, which means that SimApp is calling it: https://github.com/cosmos/cosmos-sdk/blob/release/v0.47.x/server/start.go#L256
You are right, that network.Network does not call it, but there we don't have access to the app.

[lcwik]

The app is available here and could be added to the Validator struct or returned via startInProcess so that the network would be able to call Close() on it.

[lcwik]

Also, I noticed that if I update the default implementation of BaseApp#Close to log an error and panic, neither the log statement or panic show-up in the simulator output when invoking make test-sim-multi-seed-short.

[lcwik]

I believe the issue I was seeing was that app.Close() is only invoked if tendermint node is also started. The tendermint node is not started if grpc-only is true. This leads to an early return which prevents clean-up via app.Close(), the trace writer and apiSrv further below.

If we look at startStandAlone() we can see that app.Close() would be invoked.

[julienrbrt]

True, your last point has been fixed on v0.50 and main already. I'll just add move up the defer for v0.47.

[lcwik]

Thanks