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

pkg/xds/v2: add StreamClientMutex. #885

Merged
merged 2 commits into from
Dec 27, 2019

Conversation

chainhelen
Copy link
Contributor

= assignment is not atomic in golang and partial assignments that may
occur probabilistically. ADSClient.StreamClient are both read/written
in two goroutinues, so adding mutex to guarantee concurrent security.

Issues associated with this PR

No.

Sign the CLA

Done.

Solutions

= assignment is not atomic in golang and partial assignments that may
occur probabilistically. ADSClient.StreamClient are both read/written
in two goroutinues, so adding mutex to guarantee concurrent security.

UT result

Probabilistically and theoretically.

Benchmark

Not involve.

Code Style

  • Make sure Goimports has run
    Done in goland.
  • Show Golint result
    Print nothing.

`=` assignment is not atomic in golang and partial assignments that may
occur probabilistically. `ADSClient.StreamClient` are both read/written
in two goroutinues, so adding `mutex` to guarantee concurrent security.
@chainhelen
Copy link
Contributor Author

Travis ci failed, but it seems that doesn't involve my modification.

@nejisama nejisama changed the base branch from master to stable December 23, 2019 07:45
@codecov-io
Copy link

codecov-io commented Dec 23, 2019

Codecov Report

Merging #885 into stable will decrease coverage by <.01%.
The diff coverage is 38.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           stable     #885      +/-   ##
==========================================
- Coverage   55.76%   55.76%   -0.01%     
==========================================
  Files         234      234              
  Lines       23771    23790      +19     
==========================================
+ Hits        13257    13267      +10     
- Misses       9309     9315       +6     
- Partials     1205     1208       +3
Impacted Files Coverage Δ
pkg/metrics/downstream.go 0% <ø> (ø) ⬆️
pkg/mtls/sds/subscriber.go 56.92% <0%> (ø) ⬆️
pkg/xds/v2/xdsconfig.go 0% <0%> (ø) ⬆️
pkg/router/routers_manager.go 69.69% <0%> (ø) ⬆️
pkg/stream/http/stream.go 10.24% <0%> (ø) ⬆️
pkg/xds/v2/adssubscriber.go 0% <0%> (ø) ⬆️
pkg/network/connection.go 34.13% <0%> (-0.49%) ⬇️
pkg/xds/conv/convertxds.go 25.53% <0%> (-0.04%) ⬇️
pkg/xds/conv/update.go 0% <0%> (ø) ⬆️
pkg/trace/sofa/rpc/span.go 58.33% <100%> (ø) ⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5804999...6127774. Read the comment docs.

Copy link
Contributor

@pxzero pxzero left a comment

Choose a reason for hiding this comment

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

approve

@codecov-io
Copy link

codecov-io commented Dec 27, 2019

Codecov Report

Merging #885 into master will decrease coverage by 0.02%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #885      +/-   ##
==========================================
- Coverage   55.66%   55.63%   -0.03%     
==========================================
  Files         234      234              
  Lines       23794    23802       +8     
==========================================
- Hits        13244    13243       -1     
- Misses       9336     9346      +10     
+ Partials     1214     1213       -1
Impacted Files Coverage Δ
pkg/xds/v2/adssubscriber.go 0% <0%> (ø) ⬆️
pkg/log/logger.go 55.08% <0%> (-1.61%) ⬇️
pkg/network/connection.go 33.39% <0%> (-0.37%) ⬇️
pkg/module/http2/transport.go 78.73% <0%> (+0.29%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 06f722e...109d44c. Read the comment docs.

@nejisama nejisama changed the base branch from stable to master December 27, 2019 05:28
@nejisama nejisama merged commit c478f3a into mosn:master Dec 27, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants