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

Build succeeded.

• containerd-build-arm64 : SUCCESS in 8m 05s (non-voting)

LGTM

@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

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?

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?

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

Codecov Report

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

@@            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

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.

LGTM