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: Add an Unwrap method to ConnectionError #5148

Merged
merged 1 commit into from Feb 14, 2022
+14 −0
Merged

transport: Add an Unwrap method to ConnectionError #5148

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 6 additions & 0 deletions internal/transport/transport.go
View file
Open in desktop
Expand Up @@ -741,6 +741,12 @@ func (e ConnectionError) Origin() error {
return e.err
}

// Unwrap returns the original error of this connection error or nil when the
// origin is nil.
func (e ConnectionError) Unwrap() error {
return e.err
}

var (
// ErrConnClosing indicates that the transport is closing.
ErrConnClosing = connectionErrorf(true, nil, "transport is closing")
Expand Down
8 changes: 8 additions & 0 deletions internal/transport/transport_test.go
View file
Open in desktop
Expand Up @@ -27,6 +27,7 @@ import (
"io"
"math"
"net"
"os"
"runtime"
"strconv"
"strings"
Expand Down Expand Up @@ -2399,3 +2400,10 @@ func (s) TestClientDecodeHeaderStatusErr(t *testing.T) {
})
}
}

func TestConnectionError_Unwrap(t *testing.T) {
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't test the actual usage the user (you) are expecting. Is it possible to add a test in /test at the grpc.Dial level for this, to make sure we don't break it in the future?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dfawley Sorry to see that this PR was closed. I didn't keep a close eye on it after it was approved. The test is for the actual problem. It probably affects several different use-cases.

Copy link
Member

Choose a reason for hiding this comment

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

The test is performed in an internal package, and does not go through any public APIs. So the problem is, if we stop returning transport.ConnectionError and do something different instead, whatever externally-visible use case(s) you have will be broken, but none of our tests will fail. That makes this test much less useful than something in grpc/test. Let me know if you need any help figuring out how to add a test in that package, as it can be tricky.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dfawley thanks for offering to help figuring out how to make a more encompassing test. Unfortunately, I'm in a situation where I don't have more time to spend. I created a workaround in our code when we got hit by the regression. Then, as a courtesy to you (since I really appreciate this package), I provided a PR with a fix, and a unit test to verify that the fix does what it's supposed to do. I also explained exactly how to reproduce the use-case to get the error from DialContext. I'm afraid that's as a far as I'm able to go with this, at least for the time being.

Thanks again for an excellent package.

Copy link
Member

Choose a reason for hiding this comment

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

I created a workaround in our code when we got hit by the regression.

AIUI you were relying on undocumented / unintentional behavior. This whole flow isn't something we recommend to users -- WithBlock / WithReturnConnectionError are not things we recommend using, and are not provided in most/any other languages' implementations of gRPC. As such, it is not a priority for us to maintain it. If we take this PR without an appropriate test, there's a reasonable chance it will break again in the future, and fixing it will be an extremely low priority for us, given the above. With a test, there's a much better chance we would notice it and prevent the breakage in the first place.

If you're okay with this risk and the lack of real support of this feature, we can merge this as-is. If you would like to discuss other ways of doing what you're attempting that are better supported, then we are happy to help with that as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for offering to help but we have good reasons for the approach chosen and no desire to change it at this point. I'd like you to merge this as-is.

err := connectionErrorf(false, os.ErrNotExist, "unwrap me")
if !errors.Is(err, os.ErrNotExist) {
t.Error("ConnectionError does not unwrap")
}
}