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

Handle forced connection closed by remote host error on Windows in release/1.1 branch #164

Open
austinvazquez opened this issue Mar 1, 2024 · 0 comments

Comments

@austinvazquez
Copy link
Contributor

austinvazquez commented Mar 1, 2024

Problem

When enabling CI on Windows for release/1.1 branch (#163), an edge case was found via unit tests when running on the latest Windows platform with Go 1.20+ toolchain.

=== RUN   TestOversizeCall
--- PASS: TestOversizeCall (0.03s)
=== RUN   TestClientEOF
    server_test.go:365: expected to have a cause of ErrClosed, got read unix @->TestClientEOF: wsarecv: An existing connection was forcibly closed by the remote host.
--- FAIL: TestClientEOF (0.00s)
=== RUN   TestServerRequestTimeout
--- PASS: TestServerRequestTimeout (0.01s)
=== RUN   TestServerConnectionsLeak
--- PASS: TestServerConnectionsLeak (0.10s)
=== RUN   Test_MethodFullNameGeneration
--- PASS: Test_MethodFullNameGeneration (0.00s)
FAIL
	github.com/containerd/ttrpc	coverage: 76.3% of statements
FAIL	github.com/containerd/ttrpc	1.696s
?   	github.com/containerd/ttrpc/cmd/protoc-gen-gogottrpc	[no test files]
?   	github.com/containerd/ttrpc/example	[no test files]
?   	github.com/containerd/ttrpc/example/cmd	[no test files]
?   	github.com/containerd/ttrpc/plugin	[no test files]
FAIL

The root cause is client not able to filter the connection closed by server error on Windows platform with the updated toolchain.
Ref:

ttrpc/client.go

Line 385 in 20c493e

func filterCloseErr(err error) error {

The impact is the client error does not get set to ErrClosed as expected.
Ref:

var ErrClosed = errors.New("ttrpc: closed")

Recommended Solution

The recommended solution would be to filter the error based on platform error using golang.org/x/sys package.

The solution should enable testing on Windows platform and existing TestClientEOF test should be sufficient to determine if issue is resolved.

Alternative solutions considered

Filter io.ErrClosedPIpe (Not viable)

This solution was to filter error closed on io.ErrClosedPipe.
Ref: https://github.com/containerd/ttrpc/blame/4a2816be9b4843f2cf40f1e533e7830d23f4489b/client.go#L450

Opened #165 and found this approach did not resolve the issue.

Filter based on error string

This solution would filter error closed based on the error string.

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

No branches or pull requests

1 participant