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

[release/1.1] Fix context cancelled error after client close #160

Conversation

austinvazquez
Copy link
Contributor

@austinvazquez austinvazquez commented Feb 28, 2024

Issue

#137

Description

Closes the receiver goroutine without setting the client error when the client is closed. Client error will be set on next dispatch call invoked if any.

Testing

Adds unit test which calls test service call after client is closed. Before the change, the test failed 1 in every ~3000 runs. After the change, the test passed successfully 10000 runs in a row.

Run test:

go test -timeout 300s -tags unit,integration -run ^TestClientReturnsErrClosedAfterClose$ github.com/containerd/ttrpc -v -race -count=10000

Test failure without fix:

=== RUN   TestClientReturnsErrClosedAfterClose
    client_test.go:100: Expected ErrClosed after connection has been closed, got context canceled
--- FAIL: TestClientReturnsErrClosedAfterClose (0.00s)
FAIL
FAIL    github.com/containerd/ttrpc     0.648s
FAIL

Output:

=== RUN   TestClientReturnsErrClosedAfterClose
--- PASS: TestClientReturnsErrClosedAfterClose (0.00s)
=== RUN   TestClientReturnsErrClosedAfterClose
--- PASS: TestClientReturnsErrClosedAfterClose (0.00s)
=== RUN   TestClientReturnsErrClosedAfterClose
--- PASS: TestClientReturnsErrClosedAfterClose (0.00s)
PASS
ok      github.com/containerd/ttrpc     7.583s

Alternative solutions considered

Considered setting the client error to ErrClosed when the receive goroutine exits on context cancel. I decided against this as closing the client is not an error until a call is made on a closed client which would result in the error being set to ErrClosed.

@austinvazquez austinvazquez changed the title Fix context cancelled error after client close [release/1.1] Fix context cancelled error after client close Feb 28, 2024
No longer set client error from the receiver goroutine when the client
context is closed. The client context is cancelled when the client is
closed, so the receiver goroutine can stop handling messages. The client
error will be set the next dispatch.

Signed-off-by: Austin Vazquez <[email protected]>
@austinvazquez austinvazquez force-pushed the fix-context-cancel-error-after-close branch from 57a8d60 to d2a448c Compare February 29, 2024 03:42
@dmcgowan dmcgowan merged commit f4ded63 into containerd:release/1.1 May 13, 2024
@austinvazquez austinvazquez deleted the fix-context-cancel-error-after-close branch May 16, 2024 14:00
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.

3 participants