fix: start GRPCWebServer in goroutine #9704

yihuang · 2021-07-15T10:27:47Z

so it don't block other code from executing.

Description

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)

codecov · 2021-07-15T10:36:45Z

Codecov Report

Merging #9704 (ab27b1c) into master (0027111) will increase coverage by 27.95%.
The diff coverage is 63.61%.

❗ Current head ab27b1c differs from pull request most recent head a3b81c9. Consider uploading reports for the commit a3b81c9 to get more accurate results

@@             Coverage Diff             @@
##           master    #9704       +/-   ##
===========================================
+ Coverage   35.48%   63.43%   +27.95%     
===========================================
  Files         332      573      +241     
  Lines       32620    37596     +4976     
===========================================
+ Hits        11575    23849    +12274     
+ Misses      19819    11887     -7932     
- Partials     1226     1860      +634

Impacted Files	Coverage Δ
client/keys/types.go	`100.00% <ø> (+100.00%)`	⬆️
client/query.go	`16.98% <ø> (ø)`
client/rpc/block.go	`0.00% <ø> (ø)`
client/rpc/status.go	`67.74% <ø> (ø)`
client/rpc/validators.go	`0.00% <ø> (ø)`
client/test_helpers.go	`0.00% <ø> (ø)`
client/tx/factory.go	`27.00% <ø> (ø)`
client/tx/legacy.go	`68.42% <ø> (ø)`
client/tx/tx.go	`40.83% <ø> (ø)`
client/utils.go	`41.93% <ø> (-41.40%)`	⬇️
... and 683 more

alexanderbez

LGTM.

server/grpc/grpc_web.go

fedekunze

ACK. We should push this fix to v0.43 if possible

fedekunze · 2021-07-15T14:36:37Z

@yihuang can you add a bug fix changelog entry?

yihuang · 2021-07-15T16:11:22Z

@yihuang can you add a bug fix changelog entry?

sure, done.

robert-zaremba

Thanks for contribution. Left one question.

server/grpc/grpc_web.go

robert-zaremba · 2021-07-15T18:04:57Z

server/grpc/grpc_web.go

 		return nil, err
+	case <-time.After(5 * time.Second): // assume server started successfully


Isn't 5s too long?

3 might be ok

5 seconds is also used for several other servers, do you think we should change them all, maybe share a same constant variable?

5s seams a lot, I think we can do faster, but we should check with some ops guy. Maybe @anilcse can provide some input?

robert-zaremba

We can discuss wait time for other servers in other PR. Let's merge it and we will make the release, which is more important now.

robert-zaremba · 2021-07-16T12:35:20Z

There is an import cycle, @yihuang - can you solve it please?

alexanderbez · 2021-07-16T14:27:04Z

Let's not bikeshed over the time to wait. Just pick decent value and stick with it. We can always change it later.

so it don't block other code from executing. Closes cosmos#9703 Update server/grpc/grpc_web.go Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com> add changelog entry Update server/grpc/grpc_web.go Co-authored-by: Robert Zaremba <robert@zaremba.ch> change server start time to 3 seconds and refactored to share a constant variable

yihuang · 2021-07-16T15:33:55Z

There is an import cycle, @yihuang - can you solve it please?

fixed by put the constant to server/types/app.go, that seems to be the best place?

aaronc · 2021-07-16T16:33:46Z

Sounds good to me. But there are still failing tests...

aleem1314 · 2021-07-16T18:08:29Z

I think those tests are failing because of race condition.

robert-zaremba · 2021-07-16T20:48:16Z

yes, it's failing on race detector. I can have a look at it more on Monday.

yihuang · 2021-07-16T23:53:23Z

I change it back to 5 seconds to see if the test cases pass.

alexanderbez · 2021-07-17T02:08:26Z

Could be something within ListenAndServe's logic or state that makes it potentially not thread-safe?

yihuang · 2021-07-17T08:42:36Z

Could be something within ListenAndServe's logic or state that makes potentially not thread-safe?

The grpc web server share the same grpc server handler, could that be the issue?

robert-zaremba · 2021-07-17T10:07:18Z

While looking at it I found that we don't need to wrap the grpc web handler into HTTPFunc. It doesn't solve the issue here, but simplifies the code a bit, so created this tiny PR: #9711

robert-zaremba · 2021-07-17T10:35:32Z

The race condition is in this function: https://github.com/cosmos/cosmos-sdk/blob/master/testutil/network/util.go#L25

I'm not sure if this is due to reusing the grpc handler - we are creating two different listeners for grpc and grpcWeb. They both user the same handler, but by design it should be safe to use in parallel (go http server uses many go routines go dispatches them in multiple threads).

robert-zaremba · 2021-07-17T10:41:25Z

@yihuang I have ported your changes to my branch (can't push to your branch) to do more checks for race condition. I found that we are creating unnecessary go routing in the testutil: https://github.com/cosmos/cosmos-sdk/pull/9712/files#diff-6acea81a2fe90459e23a44251e2a4fa2133e5928895472a66365e329016d9e2fL108

It solves the race problem 🍻 . If you want you can port the fix to your branch and we can merge your PR.

yihuang · 2021-07-17T17:05:43Z

@yihuang I have ported your changes to my branch (can't push to your branch) to do more checks for race condition. I found that we are creating unnecessary go routing in the testutil: https://github.com/cosmos/cosmos-sdk/pull/9712/files#diff-6acea81a2fe90459e23a44251e2a4fa2133e5928895472a66365e329016d9e2fL108

It solves the race problem 🍻 . If you want you can port the fix to your branch and we can merge your PR.

great, I added your change now.

alexanderbez · 2021-07-18T01:47:48Z

can you fix the build please @yihuang?

robert-zaremba · 2021-07-18T21:51:13Z

@yihuang you didn't correctly port the fix. Instead of:

val.grpcWeb, err :=

it should be:

val.grpcWeb, err =

Note the assignment operator := vs =.

@robert-zaremba

credit to @robert-zaremba

yihuang · 2021-07-19T01:10:11Z

@yihuang you didn't correctly port the fix. Instead of:
val.grpcWeb, err :=
it should be:
val.grpcWeb, err =
Note the assignment operator := vs =.

sorry, fixed now.

orijbot · 2021-07-19T01:21:05Z

Visit https://dashboard.github.orijtech.com?pr=9704&repo=cosmos%2Fcosmos-sdk to see benchmark details.

so it don't block other code from executing. Closes #9703  ## Description  --- ### 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... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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) (cherry picked from commit d5674a5) # Conflicts: # CHANGELOG.md

* fix: start GRPCWebServer in goroutine (#9704) so it don't block other code from executing. Closes #9703  ## Description  --- ### 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... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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) (cherry picked from commit d5674a5) # Conflicts: # CHANGELOG.md * fix changelog Co-authored-by: yihuang <huang@crypto.com> Co-authored-by: Robert Zaremba <robert@zaremba.ch>

## Description While looking into #9704 I found that we don't need to wrap grpc web handler into HTTP HandlerFun. --- ### 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... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [x] added `!` to the type prefix if API or client breaking change - [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [x] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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)

## Description While looking into #9704 I found that we don't need to wrap grpc web handler into HTTP HandlerFun. --- ### 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... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [x] added `!` to the type prefix if API or client breaking change - [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [x] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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) (cherry picked from commit 609b19a)

## Description While looking into #9704 I found that we don't need to wrap grpc web handler into HTTP HandlerFun. --- ### 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... - [x] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [x] added `!` to the type prefix if API or client breaking change - [x] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [x] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [x] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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) (cherry picked from commit 609b19a) Co-authored-by: Robert Zaremba <robert@zaremba.ch>

…9719) * fix: start GRPCWebServer in goroutine (cosmos#9704) so it don't block other code from executing. Closes cosmos#9703  ## Description  --- ### 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... - [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title - [ ] added `!` to the type prefix if API or client breaking change - [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting)) - [ ] provided a link to the relevant issue or specification - [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules) - [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing) - [ ] added a changelog entry to `CHANGELOG.md` - [ ] included comments for [documenting Go code](https://blog.golang.org/godoc) - [ ] updated the relevant documentation or specification - [ ] reviewed "Files changed" and left comments if necessary - [ ] confirmed all CI checks have passed ### 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](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) 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) (cherry picked from commit d5674a5) # Conflicts: # CHANGELOG.md * fix changelog Co-authored-by: yihuang <huang@crypto.com> Co-authored-by: Robert Zaremba <robert@zaremba.ch>

yihuang requested review from aaronc and alexanderbez as code owners July 15, 2021 10:27

yihuang changed the title ~~start GRPCWebServer in goroutine~~ fix: start GRPCWebServer in goroutine Jul 15, 2021

cyberbono3 self-assigned this Jul 15, 2021

alexanderbez approved these changes Jul 15, 2021

View reviewed changes

server/grpc/grpc_web.go Outdated Show resolved Hide resolved

fedekunze approved these changes Jul 15, 2021

View reviewed changes

fedekunze added the backport/0.43.x label Jul 15, 2021

tac0turtle approved these changes Jul 15, 2021

View reviewed changes

aaronc added the A:automerge Automatically merge PR once all prerequisites pass. label Jul 15, 2021

robert-zaremba reviewed Jul 15, 2021

View reviewed changes

robert-zaremba approved these changes Jul 16, 2021

View reviewed changes

yihuang force-pushed the fix-grpc-server branch from 4242b85 to 4a04673 Compare July 16, 2021 15:33

change back to 5 seconds

6576c72

robert-zaremba mentioned this pull request Jul 17, 2021

perf(grpcweb): remove useless HTTP HandlerFun wrapping #9711

Merged

19 tasks

robert-zaremba mentioned this pull request Jul 17, 2021

fix: start GRPCWebServer in goroutine /2 #9712

Closed

19 tasks

fix race condition in test

ab27b1c

credit to @robert-zaremba

yihuang force-pushed the fix-grpc-server branch from cf26634 to ab27b1c Compare July 19, 2021 01:06

Merge branch 'master' into fix-grpc-server

a3b81c9

mergify bot merged commit d5674a5 into cosmos:master Jul 19, 2021

mergify bot mentioned this pull request Jul 19, 2021

fix: start GRPCWebServer in goroutine (backport #9704) #9719

Merged

fedekunze mentioned this pull request Nov 14, 2021

all: cherry pick olympus mons tharsis/cosmos-sdk#1

Closed

19 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: start GRPCWebServer in goroutine #9704

fix: start GRPCWebServer in goroutine #9704

yihuang commented Jul 15, 2021 •

edited

Loading

codecov bot commented Jul 15, 2021 •

edited

Loading

alexanderbez left a comment

fedekunze left a comment

fedekunze commented Jul 15, 2021

yihuang commented Jul 15, 2021

robert-zaremba left a comment

robert-zaremba Jul 15, 2021

fedekunze Jul 15, 2021

yihuang Jul 15, 2021 •

edited

Loading

yihuang Jul 16, 2021

robert-zaremba Jul 16, 2021

robert-zaremba left a comment

robert-zaremba commented Jul 16, 2021

alexanderbez commented Jul 16, 2021

yihuang commented Jul 16, 2021

aaronc commented Jul 16, 2021

aleem1314 commented Jul 16, 2021

robert-zaremba commented Jul 16, 2021 •

edited

Loading

yihuang commented Jul 16, 2021

alexanderbez commented Jul 17, 2021 •

edited

Loading

yihuang commented Jul 17, 2021

robert-zaremba commented Jul 17, 2021

robert-zaremba commented Jul 17, 2021

robert-zaremba commented Jul 17, 2021 •

edited

Loading

yihuang commented Jul 17, 2021

alexanderbez commented Jul 18, 2021 •

edited

Loading

robert-zaremba commented Jul 18, 2021

yihuang commented Jul 19, 2021

orijbot commented Jul 19, 2021

		return nil, err
		case <-time.After(5 * time.Second): // assume server started successfully

fix: start GRPCWebServer in goroutine #9704

fix: start GRPCWebServer in goroutine #9704

Conversation

yihuang commented Jul 15, 2021 • edited Loading

Description

Author Checklist

Reviewers Checklist

codecov bot commented Jul 15, 2021 • edited Loading

Codecov Report

alexanderbez left a comment

Choose a reason for hiding this comment

fedekunze left a comment

Choose a reason for hiding this comment

fedekunze commented Jul 15, 2021

yihuang commented Jul 15, 2021

robert-zaremba left a comment

Choose a reason for hiding this comment

robert-zaremba Jul 15, 2021

Choose a reason for hiding this comment

fedekunze Jul 15, 2021

Choose a reason for hiding this comment

yihuang Jul 15, 2021 • edited Loading

Choose a reason for hiding this comment

yihuang Jul 16, 2021

Choose a reason for hiding this comment

robert-zaremba Jul 16, 2021

Choose a reason for hiding this comment

robert-zaremba left a comment

Choose a reason for hiding this comment

robert-zaremba commented Jul 16, 2021

alexanderbez commented Jul 16, 2021

yihuang commented Jul 16, 2021

aaronc commented Jul 16, 2021

aleem1314 commented Jul 16, 2021

robert-zaremba commented Jul 16, 2021 • edited Loading

yihuang commented Jul 16, 2021

alexanderbez commented Jul 17, 2021 • edited Loading

yihuang commented Jul 17, 2021

robert-zaremba commented Jul 17, 2021

robert-zaremba commented Jul 17, 2021

robert-zaremba commented Jul 17, 2021 • edited Loading

yihuang commented Jul 17, 2021

alexanderbez commented Jul 18, 2021 • edited Loading

robert-zaremba commented Jul 18, 2021

yihuang commented Jul 19, 2021

orijbot commented Jul 19, 2021

yihuang commented Jul 15, 2021 •

edited

Loading

codecov bot commented Jul 15, 2021 •

edited

Loading

yihuang Jul 15, 2021 •

edited

Loading

robert-zaremba commented Jul 16, 2021 •

edited

Loading

alexanderbez commented Jul 17, 2021 •

edited

Loading

robert-zaremba commented Jul 17, 2021 •

edited

Loading

alexanderbez commented Jul 18, 2021 •

edited

Loading