refactor: cleanup server logic by alexanderbez · Pull Request #15041 · cosmos/cosmos-sdk

alexanderbez · 2023-02-15T19:40:33Z

Description

This PR addresses the duct tape that was used during in-process application start. Specifically, we refactor the in-process logic to correctly start the gRPC and API servers, with correct graceful shutdown and signal handling. No more random clunky sleeps.

I took the approach here from the approach I've used for both Umee and Numia, i.e. use Context and ErrGroup as communication mechanisms for blocking processes, effectively forcing the caller to ensure graceful shutdown and starting via the Context.

Changelog

Ensure processes that are responsible for starting a "thing" that should block, is indeed blocking and is provided a Context that will gracefully indicate shutdown
Remove clunky sleeps
Cleanup gRPC and API server creation blocks (the if statements made no sense)

Tested

in-process w/o CPU-profiling
in-process w/ CPU-profiling
in-process w/o API and gRPC servers w/o CPU-profiling
in-process w/o API and gRPC servers w/ CPU-profiling
out-of-process w/o CPU-profiling
out-of-process w CPU-profiling

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

confirmed the correct type prefix in the PR title
confirmed ! in the type prefix if API or client breaking change
confirmed all author checklist items have been addressed
reviewed state machine logic
reviewed API design and naming
reviewed documentation is accurate
reviewed tests and test coverage
manually tested (if applicable)

+	go func(enableUnsafeCORS bool) {
+		s.logger.Info("starting API server...", "address", cfg.API.Address)

-	return tmrpcserver.Serve(s.listener, s.Router, s.logger, cmtCfg)
+		if enableUnsafeCORS {
+			allowAllCORS := handlers.CORS(handlers.AllowedHeaders([]string{"Content-Type"}))
+			errCh <- tmrpcserver.Serve(s.listener, allowAllCORS(s.Router), s.logger, cmtCfg)
+		} else {
+			errCh <- tmrpcserver.Serve(s.listener, s.Router, s.logger, cmtCfg)
+		}
+	}(cfg.API.EnableUnsafeCORS)


Wondertan · 2023-02-16T07:55:48Z

+		}
+	})
+
+	return g.Wait()


This code is unreachable, and the loop above is unnecessary.

How is it un-reachable? I've tested it and it indeed gets gracefully exits.

Wondertan · 2023-02-16T07:58:49Z

+			g.Go(func() error {
+				return servergrpc.StartGRPCServer(ctx, svrCtx.Logger.With("module", "grpc-server"), config.GRPC, grpcSrv)
+			})


So starting a server requires 2 routines long-running routines? One inside the call and the one for the caller. This can be greatly simplified

I'm not quite sure I follow?

There is the main parent process, server.go, i.e. the application process. Then there are 2 explicitly spawned goroutines, one for the API server (if enabled) and one for the gRPC server (if enabled). Both of these servers are blocking processes. Starting them in a goroutine is pretty idiomatic IMO.

What is wrong with starting the gRPC and API servers in separate goroutines? What do you see that can be cleaned up?

Wondertan · 2023-02-16T08:08:59Z

Is there any logic that handles the errors coming from Serve and other service errors reporting from eg.Wait?

(cherry picked from commit 7f99ad5) # Conflicts: # CHANGELOG.md # go.mod # go.sum # server/api/server.go # server/start.go # server/types/app.go # server/util.go # simapp/go.sum # tests/go.sum # testutil/network/network.go # testutil/network/util.go # tools/confix/go.mod # tools/confix/go.sum # x/evidence/go.mod # x/evidence/go.sum # x/feegrant/go.mod # x/feegrant/go.sum # x/nft/go.mod # x/nft/go.sum # x/upgrade/go.mod # x/upgrade/go.sum

tac0turtle · 2023-03-20T16:29:22Z

@Mergifyio backport release/v0.47.x

mergify · 2023-03-20T16:29:27Z

backport release/v0.47.x

✅ Backports have been created

Details

#15128 refactor: cleanup server logic (backport #15041) has been created for branch release/v0.47.x

cleanup for easier review

…e configurable, and add support for domain sockets for gRPC server and gRPC web server. ctx.Done() support and was added in 0.50 with cosmos#15041 server start up time was removed in 0.50 with cosmos#15041

ctx.Done() support was added in 0.50 with cosmos#15041 server start up time was removed in 0.50 with cosmos#15041

See: cosmos/cosmos-sdk#15041

ctx.Done() support was added in 0.50 with cosmos#15041 server start up time was removed in 0.50 with cosmos#15041

updates

4c97d09

github-advanced-security AI found potential problems Feb 15, 2023

View reviewed changes

Comment thread server/start.go Fixed

Comment thread server/start.go Fixed

Comment thread server/start.go Fixed

alexanderbez and others added 5 commits February 15, 2023 19:37

updates

8454b24

updates

f2d26ae

cl++

ab04e11

Merge branch 'main' into bez/14429-in-process-cleanup

8f54194

updates

6c57381

github-actions Bot added C:Confix C:Cosmovisor Issues and PR related to Cosmovisor C:x/evidence C:x/feegrant C:x/upgrade labels Feb 16, 2023

alexanderbez added the backport/v0.47.x label Feb 16, 2023

github-advanced-security AI found potential problems Feb 16, 2023

View reviewed changes

updates

4e9e264

alexanderbez marked this pull request as ready for review February 16, 2023 01:33

alexanderbez requested review from a team, likhita-809 and yihuang February 16, 2023 01:33

ghost requested review from a team, JeancarloBarrios and testinginprod and removed request for a team and likhita-809 February 16, 2023 01:33

yihuang reviewed Feb 16, 2023

View reviewed changes

Comment thread server/start.go Outdated

Wondertan reviewed Feb 16, 2023

View reviewed changes

updates

6f64b67

alexanderbez deleted the bez/14429-in-process-cleanup branch February 22, 2023 16:09

mergify Bot mentioned this pull request Feb 22, 2023

refactor: cleanup server logic (backport #15041) #15128

Closed

tac0turtle added the TODO:backport-v0.47.1 label Feb 22, 2023

tac0turtle removed the TODO:backport-v0.47.1 label Mar 20, 2023

faddat mentioned this pull request Mar 28, 2023

update linter to 1521 ChihuahuaChain/cosmos-sdk#6

Closed

19 tasks

yihuang pushed a commit to yihuang/cosmos-sdk that referenced this pull request May 18, 2023

refactor: cleanup server logic (backport cosmos#15041)

509202c

yihuang mentioned this pull request May 18, 2023

[Bug]: abci handshake not gracefully shutdown #16202

Closed

yihuang pushed a commit that referenced this pull request May 18, 2023

refactor: cleanup server logic (backport #15041)

b19f0dc

cleanup for easier review

yihuang pushed a commit to yihuang/cosmos-sdk that referenced this pull request May 18, 2023

refactor: cleanup server logic (backport cosmos#15041)

7b06bb2

cleanup for easier review

ValarDragon mentioned this pull request May 20, 2023

refactor: More cleanup of server/start.go #16238

Merged

19 tasks

larry0x pushed a commit to larry0x/cosmos-sdk that referenced this pull request May 22, 2023

refactor: cleanup server logic (cosmos#15041)

7f5799a

lcwik mentioned this pull request Oct 25, 2023

[CLOB-930] Add ctx.Done() support to shutdown, make start-up wait time configurable, and add support for domain sockets for gRPC server and gRPC web server. dydxprotocol/cosmos-sdk#29

Merged

19 tasks

lcwik added a commit to dydxprotocol/cosmos-sdk that referenced this pull request Dec 6, 2023

[CLOB-930] Add support for domain sockets for gRPC server and gRPC.

89d0d11

ctx.Done() support was added in 0.50 with cosmos#15041 server start up time was removed in 0.50 with cosmos#15041

lcwik mentioned this pull request Dec 6, 2023

[CORE-538] Apply dYdX patches to Cosmos SDK 0.50.1 fork dydxprotocol/cosmos-sdk#30

Merged

19 tasks

lcwik added a commit to dydxprotocol/cosmos-sdk that referenced this pull request Dec 6, 2023

[CLOB-930] Add support for domain sockets for gRPC server and gRPC.

2768f92

ctx.Done() support was added in 0.50 with cosmos#15041 server start up time was removed in 0.50 with cosmos#15041

lcwik added a commit to dydxprotocol/cosmos-sdk that referenced this pull request Jan 17, 2024

[CLOB-930] Add support for domain sockets for gRPC server and gRPC.

f3df409

ctx.Done() support was added in 0.50 with cosmos#15041 server start up time was removed in 0.50 with cosmos#15041

lcwik added a commit to dydxprotocol/cosmos-sdk that referenced this pull request Jan 22, 2024

[CLOB-930] Add support for domain sockets for gRPC server and gRPC.

cfe7020

ctx.Done() support was added in 0.50 with cosmos#15041 server start up time was removed in 0.50 with cosmos#15041

roy-dydx pushed a commit to dydxprotocol/cosmos-sdk that referenced this pull request Feb 20, 2024

[CLOB-930] Add support for domain sockets for gRPC server and gRPC.

7958772

ctx.Done() support was added in 0.50 with cosmos#15041 server start up time was removed in 0.50 with cosmos#15041

roy-dydx pushed a commit to dydxprotocol/cosmos-sdk that referenced this pull request Mar 26, 2024

[CLOB-930] Add support for domain sockets for gRPC server and gRPC.

5469afe

ctx.Done() support was added in 0.50 with cosmos#15041 server start up time was removed in 0.50 with cosmos#15041

tkxkd0159 mentioned this pull request Jun 13, 2024

refactor!: Remove grpc-web/rosetta and improve server performance Finschia/finschia-sdk#1418

Merged

5 tasks

lukasz-zimnoch added a commit to mezo-org/mezod that referenced this pull request Aug 7, 2024

Replace the removed server.WaitForQuitSignals function

89eae63

See: cosmos/cosmos-sdk#15041

pthmas pushed a commit to 01builders/dydx-cosmos-sdk that referenced this pull request Aug 8, 2025

[CLOB-930] Add support for domain sockets for gRPC server and gRPC.

b0c4dad

ctx.Done() support was added in 0.50 with cosmos#15041 server start up time was removed in 0.50 with cosmos#15041

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: cleanup server logic#15041

refactor: cleanup server logic#15041
alexanderbez merged 19 commits into
mainfrom
bez/14429-in-process-cleanup

alexanderbez commented Feb 15, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check notice

Uh oh!

Uh oh!

Uh oh!

Wondertan Feb 16, 2023

Uh oh!

alexanderbez Feb 16, 2023

Uh oh!

Uh oh!

Wondertan Feb 16, 2023

Uh oh!

alexanderbez Feb 16, 2023 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Wondertan commented Feb 16, 2023

Uh oh!

tac0turtle commented Mar 20, 2023

Uh oh!

mergify Bot commented Mar 20, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

alexanderbez commented Feb 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Changelog

Tested

Author Checklist

Reviewers Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Check notice

Uh oh!

Uh oh!

Uh oh!

Wondertan Feb 16, 2023

Choose a reason for hiding this comment

Uh oh!

alexanderbez Feb 16, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Wondertan Feb 16, 2023

Choose a reason for hiding this comment

Uh oh!

alexanderbez Feb 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Wondertan commented Feb 16, 2023

Uh oh!

tac0turtle commented Mar 20, 2023

Uh oh!

mergify Bot commented Mar 20, 2023

✅ Backports have been created

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

alexanderbez commented Feb 15, 2023 •

edited

Loading

alexanderbez Feb 16, 2023 •

edited

Loading