Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: data race issues with api.Server #11724

Merged

Conversation

prettymuchbryce
Copy link
Contributor

@prettymuchbryce prettymuchbryce commented Apr 21, 2022

Description

Closes: #11692

This PR adds a sync.Mutex to api.Server in order to prevent data race issues. In practice, these data races are unlikely to occur as you'd typically wait for Start to return. However, this does cause an issue with testing using the -race flag when using the testutil/network package. Currently races occur by default when running tests on projects generated using ignite. We should fix this for the sake of passing race detecting tests.


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 in the PR title
  • added ! to the type prefix if API or client breaking change
  • targeted the correct branch (see PR Targeting)
  • provided a link to the relevant issue or specification
  • followed the guidelines for building modules
  • included the necessary unit and integration tests
  • added a changelog entry to CHANGELOG.md
  • included comments for documenting Go code
  • 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 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)

@prettymuchbryce
Copy link
Contributor Author

It looks like there is a single integration test failure. Are these tests known to be occasionally flaky? I cannot reproduce the failure locally, and it's not clear to me how to re-trigger the test run.

@julienrbrt
Copy link
Member

It looks like there is a single integration test failure. Are these tests known to be occasionally flaky? I cannot reproduce the failure locally, and it's not clear to me how to re-trigger the test run.

Yes, we are working on this. Let me retry it for you 👍🏾

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK

server/api/server.go Outdated Show resolved Hide resolved
Copy link
Collaborator

@anilcse anilcse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

uACK

Copy link
Contributor

@alexanderbez alexanderbez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prettymuchbryce mind just fixing the lint/build and we can merge this? 🙏

@amaury1093 amaury1093 added the A:automerge Automatically merge PR once all prerequisites pass. label Apr 25, 2022
@codecov
Copy link

codecov bot commented Apr 25, 2022

Codecov Report

Merging #11724 (93b3002) into main (dc98d7b) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #11724   +/-   ##
=======================================
  Coverage   66.02%   66.02%           
=======================================
  Files         668      668           
  Lines       70695    70695           
=======================================
  Hits        46677    46677           
+ Misses      21327    21326    -1     
- Partials     2691     2692    +1     
Impacted Files Coverage Δ
crypto/keys/internal/ecdsa/privkey.go 82.45% <0.00%> (-1.76%) ⬇️
x/group/keeper/keeper.go 53.78% <0.00%> (+0.42%) ⬆️

@mergify mergify bot merged commit 97d735f into cosmos:main Apr 25, 2022
@alexanderbez
Copy link
Contributor

@Mergifyio backport release/v0.45.x release/v0.46.x

mergify bot pushed a commit that referenced this pull request Apr 25, 2022
## Description

Closes: #11692

This PR adds a `sync.Mutex` to `api.Server` in order to prevent data race issues. In practice, these data races are unlikely to occur as you'd typically wait for Start to return. However, this does cause an issue with testing using the `-race` flag when using the `testutil/network` package. Currently races occur by default when running tests on projects generated using `ignite`. We should fix this for the sake of passing race detecting tests.

---

### 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))
- [X] 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)
- [x] added a changelog entry to `CHANGELOG.md`
- [X] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [X] updated the relevant documentation or specification
- [x] 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 97d735f)

# Conflicts:
#	CHANGELOG.md
mergify bot pushed a commit that referenced this pull request Apr 25, 2022
## Description

Closes: #11692

This PR adds a `sync.Mutex` to `api.Server` in order to prevent data race issues. In practice, these data races are unlikely to occur as you'd typically wait for Start to return. However, this does cause an issue with testing using the `-race` flag when using the `testutil/network` package. Currently races occur by default when running tests on projects generated using `ignite`. We should fix this for the sake of passing race detecting tests.

---

### 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))
- [X] 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)
- [x] added a changelog entry to `CHANGELOG.md`
- [X] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [X] updated the relevant documentation or specification
- [x] 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 97d735f)

# Conflicts:
#	CHANGELOG.md
@mergify
Copy link
Contributor

mergify bot commented Apr 25, 2022

backport release/v0.45.x release/v0.46.x

✅ Backports have been created

@prettymuchbryce
Copy link
Contributor Author

Thanks @AmauryM @alexanderbez @anilcse @julienrbrt 🙏

dwedul-figure added a commit to provenance-io/cosmos-sdk that referenced this pull request May 19, 2022
* check error returned from NewNode (cosmos#11624)

* docs: Add v0.45.x to docs versions (backport cosmos#11613) (cosmos#11623)

* docs: Update mentions of Starport to Ignite CLI (backport cosmos#11612) (cosmos#11617)

* feat: add (re)delegation getters (backport cosmos#11596) (cosmos#11615)

* feat: add (re)delegation getters (cosmos#11596)

### Description

This PR adds general helper functions to the `x/staking` module that are used in the Evmos `x/vesting` module and originated from Agoric's custom staking module implementation https://github.com/agoric-labs/cosmos-sdk/blob/4085-true-vesting/x/staking/keeper/delegation.go.

- `GetDelegatorUnbonding`
- `GetDelegatorBonded`
- `IterateDelegatorUnbondingDelegations`
- `IterateDelegatorDelegations`
- `IterateDelegatorRedelegations`

(cherry picked from commit b8270fc)

# Conflicts:
#	CHANGELOG.md
#	x/staking/keeper/delegation.go

* fix RemoveDelegation method conflict

* fix CHANGELOG conflict

* add new PR in CHANGELOG

Co-authored-by: Daniel Burckhardt <daniel.m.burckhardt@gmail.com>

* feat: add vesting util functions (backport cosmos#11652) (cosmos#11667)

* feat: add vesting util functions (cosmos#11652)

* feat: add vesting util functions

* changelog

* revert string deletion

* fix build

* Update x/auth/vesting/types/period.go

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>

Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
Co-authored-by: Marko <marbar3778@yahoo.com>
(cherry picked from commit c676952)

# Conflicts:
#	CHANGELOG.md
#	x/auth/vesting/types/period.go

* changelog

* Update x/auth/vesting/types/period.go

Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
Co-authored-by: Federico Kunze Küllmer <federico.kunze94@gmail.com>

* chore: v0.45.x bump min go version to 1.17 (cosmos#11686)

* fix: Add validation on create gentx (backport cosmos#11693) (cosmos#11698)

* fix: Added description of how to run the unsafe-reset-all command in simapp README (cosmos#11718) (cosmos#11719)

* Fix simapp README

* Revised review content

Co-authored-by: Julien Robert <julien@rbrt.fr>
(cherry picked from commit d5e0b86)

Co-authored-by: Takahiko Tominaga <57429437+takapi327@users.noreply.github.com>

* chore: remove unneeded swagger docs from 0.45 cosmos#11745

* fix: data race issues with api.Server (backport cosmos#11724) (cosmos#11748)

* updates (cosmos#11750)

* updates (cosmos#11752)

* chore: release notes++

* Add dynamic build tag so this can build on M1 macs.

* Change README line endings to dos since that's what they were originally.

Co-authored-by: Jorge Hernandez <3452489+jhernandezb@users.noreply.github.com>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
Co-authored-by: Daniel Burckhardt <daniel.m.burckhardt@gmail.com>
Co-authored-by: Federico Kunze Küllmer <31522760+fedekunze@users.noreply.github.com>
Co-authored-by: Federico Kunze Küllmer <federico.kunze94@gmail.com>
Co-authored-by: Aleksandr Bezobchuk <alexanderbez@users.noreply.github.com>
Co-authored-by: Takahiko Tominaga <57429437+takapi327@users.noreply.github.com>
Co-authored-by: Marko <marbar3778@yahoo.com>
Co-authored-by: Aleksandr Bezobchuk <aleks.bezobchuk@gmail.com>
JimLarson pushed a commit to agoric-labs/cosmos-sdk that referenced this pull request Jul 7, 2022
randy75828 pushed a commit to Switcheo/cosmos-sdk that referenced this pull request Aug 10, 2022
JeancarloBarrios pushed a commit to agoric-labs/cosmos-sdk that referenced this pull request Sep 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A:automerge Automatically merge PR once all prerequisites pass.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Data race when using testutil/network
5 participants