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

Add support to gRPC errdefs for context cancel/deadline exceeded #3308

Merged

Conversation

jterry75
Copy link
Contributor

Signed-off-by: Justin Terry (VM) juterry@microsoft.com

Signed-off-by: Justin Terry (VM) <juterry@microsoft.com>
@jterry75 jterry75 requested a review from Random-Liu May 29, 2019 00:02
@jterry75
Copy link
Contributor Author

@Random-Liu - Will update: containerd/cri#1152 when this is merged.

@theopenlab-ci
Copy link

theopenlab-ci bot commented May 29, 2019

Build succeeded.

Copy link
Member

@cpuguy83 cpuguy83 left a comment

Choose a reason for hiding this comment

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

LGTM

@Random-Liu
Copy link
Member

LGTM

@Random-Liu
Copy link
Member

--- FAIL: TestClientTTRPC_Close (0.01s)
    client_ttrpc_test.go:70: assertion failed: rpc error: code = InvalidArgument desc = event envelope has invalid namespace: namespace "" must match ^[A-Za-z][A-Za-z0-9]+(:?[-]+[A-Za-z][A-Za-z0-9]+)*(?:[.](?:[A-Za-z][A-Za-z0-9]+(:?[-]+[A-Za-z][A-Za-z0-9]+)*))*$: invalid argument (err *status.statusError) != ttrpc: closed (ttrpc.ErrClosed *errors.fundamental)

@jterry75
Copy link
Contributor Author

@crosbymichael this failed at:

--- FAIL: TestClientTTRPC_Close (0.01s)
    client_ttrpc_test.go:70: assertion failed: rpc error: code = InvalidArgument desc = event envelope has invalid namespace: namespace "" must match ^[A-Za-z][A-Za-z0-9]+(:?[-]+[A-Za-z][A-Za-z0-9]+)*(?:[.](?:[A-Za-z][A-Za-z0-9]+(:?[-]+[A-Za-z][A-Za-z0-9]+)*))*$: invalid argument (err *status.statusError) != ttrpc: closed (ttrpc.ErrClosed *errors.fundamental)

Looks like the same test passed in the async tests. Is there a race here? Seems unrelated but maybe we should investigate and fix

@fuweid
Copy link
Member

fuweid commented May 29, 2019

Looks like the same test passed in the async tests. Is there a race here? Seems unrelated but maybe we should investigate and fix

hi @jterry75

I meet this issue yesterday. I found the it is related to select. https://github.com/containerd/ttrpc/blob/master/client.go#L203-L245
If we close the client and send the request, and the select is not scheduled, it will randomly pick one input. does it make senses?

@jterry75
Copy link
Contributor Author

jterry75 commented May 29, 2019

Looks like the same test passed in the async tests. Is there a race here? Seems unrelated but maybe we should investigate and fix

hi @jterry75

I meet this issue yesterday. I found the it is related to select. https://github.com/containerd/ttrpc/blob/master/client.go#L203-L245
If we close the client and send the request, and the select is not scheduled, it will randomly pick one input. does it make senses?

@fuweid - I think you are on to something here. Just reviewed the code. It does look like its mostly correct. Its a race for sure. I wonder if client.Close() should wait for c.done so that we know the run() func returns. That way there would be no possible way to issue a write to Call.

Although it looks like c.calls is actually never closed (bug?). And it might just be that we should have dispatch forcibly check for c.done rather than a select.

Ideas?

@jterry75
Copy link
Contributor Author

Given that the issue is not related to this PR can we do them as separate PR's?

@codecov-io
Copy link

Codecov Report

Merging #3308 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3308      +/-   ##
==========================================
+ Coverage    44.6%   44.66%   +0.05%     
==========================================
  Files         112      112              
  Lines       12180    12192      +12     
==========================================
+ Hits         5433     5445      +12     
  Misses       5913     5913              
  Partials      834      834
Flag Coverage Δ
#linux 48.56% <100%> (+0.06%) ⬆️
#windows 39.94% <100%> (+0.07%) ⬆️
Impacted Files Coverage Δ
errdefs/errors.go 100% <100%> (ø) ⬆️
errdefs/grpc.go 77.5% <100%> (+2.5%) ⬆️

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 0e7a3c9...ac4485c. Read the comment docs.

1 similar comment
@codecov-io
Copy link

Codecov Report

Merging #3308 into master will increase coverage by 0.05%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3308      +/-   ##
==========================================
+ Coverage    44.6%   44.66%   +0.05%     
==========================================
  Files         112      112              
  Lines       12180    12192      +12     
==========================================
+ Hits         5433     5445      +12     
  Misses       5913     5913              
  Partials      834      834
Flag Coverage Δ
#linux 48.56% <100%> (+0.06%) ⬆️
#windows 39.94% <100%> (+0.07%) ⬆️
Impacted Files Coverage Δ
errdefs/errors.go 100% <100%> (ø) ⬆️
errdefs/grpc.go 77.5% <100%> (+2.5%) ⬆️

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 0e7a3c9...ac4485c. Read the comment docs.

@crosbymichael
Copy link
Member

LGTM

@crosbymichael crosbymichael merged commit 7451dd1 into containerd:master Jun 3, 2019
@jterry75 jterry75 deleted the handle_grpc_context_error branch June 3, 2019 20:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants