go-spacemesh
go-spacemesh copied to clipboard
spacemeshos
Closing API streams
Description
Each of the API Stream endpoints uses a select{} statement to poll for incoming events to report, and to check if the channels that it's listening to are closed. Right now this is done rather naively: if any of the channels it's listening on are closed, the entire endpoint returns and closes the stream. (The GRPC service can also pass in a ctx.Done, in which case it closes, but I think this would be passed from the remote end of the stream, not from the local node itself.)
- Do we want to close the entire stream if only one of multiple channels closes? (in practice this will probably never happen - they will all be closed together when the node shuts down)
- Do we want to add another case to the
select{}to listen for a context.Done being passed down from an outer context? (in practice this is probably not necessary since, when the node shuts down, the channels will be shut down, so it will receive the signal that way, but I don't know enough about golang to understand if this risks leaking a channel)
Affected code
All of the *Stream endpoints in the API, e.g.:
https://github.com/spacemeshos/go-spacemesh/blob/aa7eb73e121e7746de3468d1a365523dc729e3fb/api/grpcserver/globalstate_service.go#L282-L287
@howydev before trying to fix this, I think we need to answer this question:
Do we want to add another case to the select{} to listen for a context.Done being passed down from an outer context? (in practice this is probably not necessary since, when the node shuts down, the channels will be shut down, so it will receive the signal that way, but I don't know enough about golang to understand if this risks leaking a channel)
We need to understand how the "kill" signal propagates down from the top-level App down to the API channel listeners.
@lrettig Makes sense, I'll spend time looking into that before continuing. I assume there's no implementation/usage of the API thus far?
I assume there's no implementation/usage of the API thus far?
Not sure what you mean - you can run the API on current testnet nodes. We have public API nodes running, and the explorer and dash are using it too!
Yes, but low priority/nice to have. We should make sure the API closes cleanly in all scenarios.