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.

Sign up for GitHub

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

Jump to bottom

transport: do not close channel that can lead to panic #2695

Merged
merged 1 commit into from
Mar 19, 2019
Merged

transport: do not close channel that can lead to panic #2695

merged 1 commit into from
Mar 19, 2019

Conversation

aanm
Copy link
Contributor

@aanm aanm commented Mar 18, 2019

Write can be called concurrently, for which it calls the do function.
As WriteStatus can close the ht.writes in parallel as well the Write
will try to write into the ht.writes in the do function, this can
lead into a panic. As there is no real usability on closing this channel
we can simply leave it to the garbage collector so we can avoid panic
during an execution.

Signed-off-by: André Martins aanm90@gmail.com

@aanm
Copy link
Contributor Author

aanm commented Mar 18, 2019

@gyuho We were hitting this [0] panic a couple of times and once we have applied this hot fix in etcd v3.3.11 and we haven't gotten any panic so far.

[0] etcd-io/etcd#9956

menghanl
menghanl reviewed Mar 18, 2019
View reviewed changes
Copy link
Contributor

@menghanl menghanl left a comment

Choose a reason for hiding this comment

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

LGTM.

Can you also delete ok in runStream()? It will never be false since writes is never closed. Thanks!

@aanm
Copy link
Contributor Author

aanm commented Mar 18, 2019

@menghanl Done!

@aanm
transport: do not close channel that can lean to panic
589eb9d 
`Write` can be called concurrently, for which it calls the `do` function.
As `WriteStatus` can close the `ht.writes` in parallel as well the `Write`
will try to write into the `ht.writes` in the `do` function, this can
lead into a panic. As there is no real usability on closing this channel
we can simply leave it to the garbage collector so we can avoid panic
during an execution.

Signed-off-by: André Martins <aanm90@gmail.com>
@aanm
Copy link
Contributor Author

aanm commented Mar 19, 2019

It seems master branch is also failing on the same test

menghanl
menghanl approved these changes Mar 19, 2019
View reviewed changes
Copy link
Contributor

@menghanl menghanl left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@menghanl menghanl merged commit 272a4b6 into grpc:master Mar 19, 2019
@dfawley dfawley changed the title transport: do not close channel that can lean to panic transport: do not close channel that can lead to panic Mar 22, 2019
@dfawley dfawley added this to the 1.20 Release milestone Mar 22, 2019
@xiang90 xiang90 mentioned this pull request Mar 28, 2019
Closed
aanm added a commit to aanm/etcd that referenced this pull request Apr 10, 2019
@aanm
vendor: update grpc-go to v1.20.0o
c46837e 
This version includes patch from
grpc/grpc-go#2695 which
fixes etcd-io#9956

Signed-off-by: André Martins <aanm90@gmail.com>
aanm added a commit to aanm/etcd that referenced this pull request Apr 10, 2019
@aanm
vendor: update grpc-go to v1.20.0
d1ecbac 
This version includes patch from
grpc/grpc-go#2695 which
fixes etcd-io#9956

Signed-off-by: André Martins <aanm90@gmail.com>
@aanm aanm mentioned this pull request Apr 10, 2019
Closed
@tonistiigi tonistiigi mentioned this pull request Apr 10, 2019
Closed
@thaJeztah thaJeztah mentioned this pull request Apr 11, 2019
Merged
@thaJeztah thaJeztah mentioned this pull request Apr 11, 2019
Merged
thaJeztah added a commit to thaJeztah/cli that referenced this pull request Apr 12, 2019
@thaJeztah
bump google.golang.org/grpc v1.20.0
e36d10c 
full diff: grpc/grpc-go@v1.12.2...v1.20.0

includes  grpc/grpc-go#2695 transport: do not close channel that can lead to panic
addresses moby/moby#39053

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah mentioned this pull request Apr 12, 2019
Merged
thaJeztah added a commit to thaJeztah/cli that referenced this pull request Apr 12, 2019
@thaJeztah
bump google.golang.org/grpc v1.20.0
b60ce1a 
full diff: grpc/grpc-go@v1.12.2...v1.20.0

includes  grpc/grpc-go#2695 transport: do not close channel that can lead to panic
addresses moby/moby#39053

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
aanm added a commit to aanm/etcd that referenced this pull request Apr 23, 2019
@aanm
vendor: update grpc-go to v1.20.0
495d3ab 
This version includes patch from
grpc/grpc-go#2695 which
fixes etcd-io#9956

Signed-off-by: André Martins <aanm90@gmail.com>
aanm added a commit to aanm/etcd that referenced this pull request May 13, 2019
@aanm
vendor: update grpc-go to v1.20.0
686c9f7 
This version includes patch from
grpc/grpc-go#2695 which
fixes etcd-io#9956

Signed-off-by: André Martins <aanm90@gmail.com>
thaJeztah added a commit to thaJeztah/cli that referenced this pull request May 13, 2019
@thaJeztah
bump google.golang.org/grpc v1.20.1
93d76c5 
full diff: grpc/grpc-go@v1.12.2...v1.20.1

includes  grpc/grpc-go#2695 transport: do not close channel that can lead to panic
addresses moby/moby#39053

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah added a commit to thaJeztah/docker that referenced this pull request May 13, 2019
@thaJeztah
bump google.golang.org/grpc v1.20.1
28ad54d 
full diff: grpc/grpc-go@v1.12.2...v1.20.1

includes  grpc/grpc-go#2695 transport: do not close channel that can lead to panic
addresses moby#39053

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah added a commit to thaJeztah/docker that referenced this pull request May 14, 2019
@thaJeztah
bump google.golang.org/grpc v1.20.1
d371b28 
full diff: grpc/grpc-go@v1.12.2...v1.20.1

includes  grpc/grpc-go#2695 transport: do not close channel that can lead to panic
addresses moby#39053

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 28ad54d)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
thaJeztah added a commit to thaJeztah/cli that referenced this pull request May 14, 2019
@thaJeztah
bump google.golang.org/grpc v1.20.1
57ef4e3 
full diff: grpc/grpc-go@v1.12.2...v1.20.1

includes  grpc/grpc-go#2695 transport: do not close channel that can lead to panic
addresses moby/moby#39053

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 93d76c5)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@thaJeztah thaJeztah mentioned this pull request May 14, 2019
Merged
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request May 14, 2019
@thaJeztah
bump google.golang.org/grpc v1.20.1
e57ff1e 
full diff: grpc/grpc-go@v1.12.2...v1.20.1

includes  grpc/grpc-go#2695 transport: do not close channel that can lead to panic
addresses moby/moby#39053

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Upstream-commit: 93d76c5c9017b8146dcac5f4106e10ba385cce4d
Component: cli
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request May 14, 2019
@thaJeztah
bump google.golang.org/grpc v1.20.1
5b4d325 
full diff: grpc/grpc-go@v1.12.2...v1.20.1

includes  grpc/grpc-go#2695 transport: do not close channel that can lead to panic
addresses moby/moby#39053

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 28ad54d84f05b789e6a5f9a2ff2a5230d2ec886e)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Upstream-commit: d371b283c3dd3319350caf23336d6bc93bd5a15d
Component: engine
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request May 14, 2019
@thaJeztah
bump google.golang.org/grpc v1.20.1
717821e 
full diff: grpc/grpc-go@v1.12.2...v1.20.1

includes  grpc/grpc-go#2695 transport: do not close channel that can lead to panic
addresses moby/moby#39053

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
(cherry picked from commit 93d76c5c9017b8146dcac5f4106e10ba385cce4d)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Upstream-commit: 57ef4e32f4ee7fac888c5749312f7aa23a11ab7b
Component: cli
docker-jenkins pushed a commit to docker-archive/docker-ce that referenced this pull request May 14, 2019
@thaJeztah
bump google.golang.org/grpc v1.20.1
a807234 
full diff: grpc/grpc-go@v1.12.2...v1.20.1

includes  grpc/grpc-go#2695 transport: do not close channel that can lead to panic
addresses moby/moby#39053

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Upstream-commit: 28ad54d84f05b789e6a5f9a2ff2a5230d2ec886e
Component: engine
kiku-jw pushed a commit to kiku-jw/moby that referenced this pull request May 16, 2019
@thaJeztah @kiku-jw
bump google.golang.org/grpc v1.20.1
9f67a06 
full diff: grpc/grpc-go@v1.12.2...v1.20.1

includes  grpc/grpc-go#2695 transport: do not close channel that can lead to panic
addresses moby#39053

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
aanm added a commit to aanm/etcd that referenced this pull request May 18, 2019
@aanm
vendor: update grpc-go to v1.20.0
9e1bdca 
This version includes patch from
grpc/grpc-go#2695 which
fixes etcd-io#9956

Signed-off-by: André Martins <aanm90@gmail.com>
@lock lock bot locked as resolved and limited conversation to collaborators Sep 18, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@menghanl menghanl menghanl approved these changes

Assignees
No one assigned
Labels
Type: Bug
Projects
None yet
Milestone
1.20 Release
Development

Successfully merging this pull request may close these issues.
3 participants
@aanm @menghanl @dfawley