refactor: cleanup server logic #15041

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)

server/start.go

server/api/server.go

+	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)


server/start.go

server/grpc/server.go

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

server/start.go

+		}
+	})
+
+	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.

server/api/server.go

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

server/start.go

+			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?

server/grpc/server.go

server/start.go

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?

alexanderbez · 2023-02-16T15:19:36Z

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

The eg.Wait() is in the command-handler for in-process ABCI server spawning. That error is returned to the cobra command which is then returned to the calling process.

(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

#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

updates

4c97d09

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

View reviewed changes

server/start.go Fixed Show fixed Hide fixed

server/start.go Fixed Show fixed Hide fixed

server/start.go Fixed Show fixed Hide 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 Issues and PR related to Confix C:Cosmovisor Issues and PR related to Cosmovisor C:Rosetta Issues and PR related to Rosetta C:x/evidence C:x/feegrant C:x/nft C:x/upgrade labels Feb 16, 2023

alexanderbez added the backport/v0.47.x PR scheduled for inclusion in the v0.47's next stable release label Feb 16, 2023

github-advanced-security bot 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 a review from a team as a code owner February 16, 2023 01:33

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

yihuang reviewed Feb 16, 2023

View reviewed changes

server/start.go Outdated Show resolved Hide resolved

Wondertan reviewed Feb 16, 2023

View reviewed changes

updates

6f64b67

alexanderbez added 3 commits February 16, 2023 10:40

updates

08a31ae

updates

e7c165d

updates

22fd738

alexanderbez requested a review from Wondertan February 16, 2023 15:51

updates

425db71

alexanderbez merged commit 7f99ad5 into main Feb 22, 2023

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.

234b9f7

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: cleanup server logic #15041

refactor: cleanup server logic #15041

alexanderbez commented Feb 15, 2023 •

edited

Loading

Wondertan Feb 16, 2023

alexanderbez Feb 16, 2023

Wondertan Feb 16, 2023

alexanderbez Feb 16, 2023 •

edited

Loading

Wondertan commented Feb 16, 2023

alexanderbez commented Feb 16, 2023

tac0turtle commented Mar 20, 2023

mergify bot commented Mar 20, 2023

refactor: cleanup server logic #15041

refactor: cleanup server logic #15041

Conversation

alexanderbez commented Feb 15, 2023 • edited Loading

Description

Changelog

Tested

Author Checklist

Reviewers Checklist

Wondertan Feb 16, 2023

Choose a reason for hiding this comment

alexanderbez Feb 16, 2023

Choose a reason for hiding this comment

Wondertan Feb 16, 2023

Choose a reason for hiding this comment

alexanderbez Feb 16, 2023 • edited Loading

Choose a reason for hiding this comment

Wondertan commented Feb 16, 2023

alexanderbez commented Feb 16, 2023

tac0turtle commented Mar 20, 2023

mergify bot commented Mar 20, 2023

✅ Backports have been created

alexanderbez commented Feb 15, 2023 •

edited

Loading

alexanderbez Feb 16, 2023 •

edited

Loading