Use errors.Is() in IsErrEOF() #8698

pierreca · 2020-09-17T01:52:05Z

IsErrEOF returns false when it should return true in a couple of cases:

if the error has been wrapped in another error (for example, if EOF
is wrapped in an RPC error)
if the error has been created from an Error field in an RPC response
(as it is the case in CallWithCodec in the net-rpc-msgpackrpc package
for example)

IsErrEOF returns false when it should return true in a couple of cases: 1. if the error has been wrapped in another error (for example, if EOF is wrapped in an RPC error) 2. if the error has been created from an Error field in an RPC response (as it is the case in CallWithCodec in the net-rpc-msgpackrpc package for example)

hashicorp-cla · 2020-09-17T01:52:07Z

All committers have signed the CLA.

dnephin

Thank you for this PR!

I think using errors.Is is the right thing to do here, but some tests for this change would be great! A table test with some different cases seems appropriate.

One concern inline below.

dnephin · 2020-09-17T15:58:00Z

lib/eof.go

 		return true
 	}

 	errStr := err.Error()
 	if strings.Contains(errStr, yamuxStreamClosed) ||
-		strings.Contains(errStr, yamuxSessionShutdown) {
+		strings.Contains(errStr, yamuxSessionShutdown) ||
+		strings.HasSuffix(errStr, io.EOF.Error()) {


In theory this seems reasonable, but I think because EOF.Error() is such a short string (EOF), there is a pretty significant chance this incorrectly matches a a different error.

A suffix of : EOF might be a bit safer, but I'm not sure that is necessary either. I think we should be encouraging all code to use wrapping and not rely on string comparison. Ideally we would remove the need for these other strings.Contains() as well.

I think we should not add this strings.HasSuffix, and fix any code that does not properly wrap errors to do so.

I agree on principle and long-term that's what i'd want too.

To give some context on the string comparison, the code that motivated it is here:
https://github.com/hashicorp/net-rpc-msgpackrpc/blob/master/client.go#L33

In this case, the error comes in the Response.Error field as a string, and the string is copied into an error object. In the specific case I am seeing the string is: rpc error: EOF.
This error created by this line of code does not unwrap correctly as an EOF and as far as I can tell, cannot be identified as an EOF other than by string comparison. Maybe there's a quick fix I don't know about and if that's the case I'll happily submit another PR for it, but if there isn't i'm afraid the bigger fix involves changing how the RPC client works, and If I understand the situation correctly, this is used in many places in many hashicorp projects. Which makes this change really scary for a first time external contributor.

Thoughts?

Ah, yes I see the problem. Do you know where the rpc error: prefix comes from? I'm not seeing anywhere that would append an EOF.

I think if there are specific cases like this that we know can not be unwrapped, we could first check that the error is a specific type, then check the string, something like

if errors.Is(err, rpc.ServerError{}) && strings.HasSuffix(err.Error(), ': EOF') {}

I think if we can track down the specific cases where this is a problem we can at least be very struct about the comparison.

I really like this idea, I'll send an update.

The scenario i'm testing is on long-poll requests where a user of the API is connected to a nomad client, the client is forwarding the RPC to a server, and while the request is blocked, the leader is killed. In that specific case, sometimes, I see this wrapped error as string. I think it may have to do with how the requests are forwarded between the client, its server, and potentially the cluster leader. I'm still learning how that codepath works, I've only been staring at it for two days :)

What can I do that would help narrow this down? I can repro on a fairly regular basis, I just need help orienting myself in the code and adding logs where we need them to confirm the problematic code path(s).

I have added the ServerError type assertion: I hope it is restrictive enough and solves at least one of the failure mode I'm trying to get past (explained below, step 3). I'm not out of the woods yet but if this gets checked in, I hope it'll help improve robustness.

Long explanation
I am starting to understand that the problem I'm trying to solve goes deeper and lies in the forwarding chain of the RPC requests in a nomad cluster. Here's the scenario:

api user makes a blocking request (wait on an index) -> nomad client -> nomad server (leader)

leader dies/is killed while the request is waiting. Returns an EOF error

with this fix: client retries and changes server: client -> nomad server(follower) (without the fix, the client would not understand it received an EOF error and would not retry, that's the improvement)

nomad server (follower) -> nomad server(leader) connection refused: the leader (that died in step 2) is down and new leader election hasn't happened yet

follower does not retry and passes the error (connection refused) down to the client in the response.Error field

client: gets wrapped version of the connection refused error and since it's not an EOF error, passes it down to the caller <- that's my new problem :)

From the caller perspective this is difficult, if not impossible, to handle correctly: the caller shouldn't be expected to understand (and recover from) RPC forwarding chain errors. There are a finite number of scenarios like this (depending whether the client is connected to a follower or a leader, how fast leader election goes, and which server goes down, the one doing the forwarding or the leader). I think this should be entirely handled within the RPC chain between the nomad client and servers. retries and HA are hard problems though :)

This being said, I don't think it's realistic to expect that all cases can be covered with IsErrEOF(), which is already a little bit extended past EOF (the yamuxError, and now some ServerErrors provided they end with : EOF). The follow up investigation is probably directly in the nomad RPC code.

In conclusion, I still think this is a net improvement and we've pushed this PR as far as it should go, happy to make more changes as required to get it merged, this is a move in the right direction :)

dnephin · 2020-09-17T23:41:37Z

lib/eof.go

 		return true
 	}

 	errStr := err.Error()
 	if strings.Contains(errStr, yamuxStreamClosed) ||
-		strings.Contains(errStr, yamuxSessionShutdown) {
+		strings.Contains(errStr, yamuxSessionShutdown) ||
+		strings.HasSuffix(errStr, io.EOF.Error()) {


Ah, yes I see the problem. Do you know where the rpc error: prefix comes from? I'm not seeing anywhere that would append an EOF.

I think if there are specific cases like this that we know can not be unwrapped, we could first check that the error is a specific type, then check the string, something like

if errors.Is(err, rpc.ServerError{}) && strings.HasSuffix(err.Error(), ': EOF') {}

I think if we can track down the specific cases where this is a problem we can at least be very struct about the comparison.

dnephin · 2020-09-18T01:01:49Z

lib/eof_test.go

+)
+
+func TestErrIsEOF(t *testing.T) {
+	t.Parallel()


I know a lot of the tests use t.Parallel, but it can cause problems sometimes so I've been trying to remove it where possible. I think in this case, since these tests run so fast, it's actually slower to spin up the goroutine to run the tests in parallel, than it would be to run it without parallel.

Suggested change

t.Parallel()

dnephin · 2020-09-18T01:01:59Z

lib/eof_test.go

+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			t.Parallel()


Suggested change

t.Parallel()

dnephin · 2020-09-18T01:02:03Z

lib/eof_test.go

+			require.True(t, IsErrEOF(tt.err))
+		})
+		t.Run(fmt.Sprintf("Wrapped %s", tt.name), func(t *testing.T) {
+			t.Parallel()


Suggested change

t.Parallel()

dnephin · 2020-09-18T01:02:08Z

lib/eof_test.go

+			require.True(t, IsErrEOF(fmt.Errorf("test: %w", tt.err)))
+		})
+		t.Run(fmt.Sprintf("String suffix is %s", tt.name), func(t *testing.T) {
+			t.Parallel()


Suggested change

t.Parallel()

pierreca · 2020-09-21T21:17:06Z

@dnephin would you mind giving this another look and let me know if these last commits address everything you wanted?

dnephin

I think this implementation is looking good, but I realized I still may not fully understand the specific problem this is fixing. I need to read over your detailed comment above again more carefully to make sure I understand. My main concern is that this IsErrEOF function is called from a few places, and I'm not sure if this change will be correct for all of the places.

lib/eof.go

pierreca · 2020-09-24T19:43:24Z

@dnephin no worries, I understand, i've been on the maintainer side for years in the past. it's hard. happy to jump in a slack conversation or something like that if it makes it easier.

The bottom line is that I have found that the nomad retry logic is broken in rpc forwarding scenarios and that part of the problem is not all EOF errors aren't properly identified as such. What this change does is widen the scope of what gets recognized as EOF to include wrapped io.EOF errors and EOF errors wrapped in ServerError that are sent in the RPC response body.

What are the risks?

interpreting an error that isn't an EOF as an EOF: this was your original concern, I believe it's addressed by narrowing it down to precise type checking
kicking in retries where the code would not retry before, which has 2 possibilities:
- it should've retried (that's a bugfix then, and that's what I'm going for and seeing in my tests)
- it shouldn't have retried: that'd be either a bad interpretation of the error or a really weird case (ok to retry on EOF but not wrapped EOF is a weird scenario IMHO). It should be fairly easy to review those few cases just by looking at where IsErrEOF is used:

IDK what your test coverage looks like but if there are tests I can write to help you make sure this doesn't break, just point me where. I think there is one or two more changes needed in nomad after that to fix retries in RPC forwarding chains and that'd be a win for a lot of users :)

pierreca · 2020-10-05T18:13:25Z

@dnephin ping? anything else I can do to help get this checked in? we hit errors related to nomad not retrying when it should very consistently. there's more work that needs to happen to fix the problems but this is at the root of a lot of missed retries.

pierreca · 2020-10-14T01:16:08Z

@dnephin still hoping to upstream this, lmk if I can help address any doubt that this is a good fix.

dnephin

Thank you for the fix!

One suggestion for some additional test cases, but not blocking.

dnephin · 2021-03-16T21:55:48Z

lib/eof_test.go

+		{name: "EOF", err: io.EOF},
+		{name: "Wrapped EOF", err: fmt.Errorf("test: %w", io.EOF)},
+		{name: "yamuxStreamClosed", err: yamux.ErrStreamClosed},
+		{name: "yamuxSessionShutdown", err: yamux.ErrSessionShutdown},
+		{name: "ServerError(___: EOF)", err: rpc.ServerError(fmt.Sprintf("rpc error: %s", io.EOF.Error()))},
+		{name: "Wrapped ServerError(___: EOF)", err: fmt.Errorf("rpc error: %w", rpc.ServerError(fmt.Sprintf("rpc error: %s", io.EOF.Error())))},


A couple tests that are expected to return false might be good as well, to check that errors which are not EOF are not matched.

hashicorp-ci · 2021-03-16T21:57:03Z

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/338197.

hashicorp-ci · 2021-03-16T21:57:07Z

🍒✅ Cherry pick of commit 931023f onto release/1.9.x succeeded!

Use errors.Is() in IsErrEOF()

hashicorp-ci · 2021-03-16T21:57:11Z

🍒✅ Cherry pick of commit 931023f onto release/1.8.x succeeded!

Use errors.Is() in IsErrEOF()

hashicorp-ci · 2021-03-16T21:57:14Z

🍒✅ Cherry pick of commit 931023f onto release/1.7.x succeeded!

Use errors.Is() in IsErrEOF()

pierreca mentioned this pull request Sep 17, 2020

Use errors.Is() in IsErrEOF() hashicorp/nomad#8910

Closed

platttammy approved these changes Sep 17, 2020

View reviewed changes

dnephin reviewed Sep 17, 2020

View reviewed changes

dnephin added the theme/internal-cleanup Used to identify tech debt, testing improvements, code refactoring, and non-impactful optimization label Sep 17, 2020

Add unit tests for isErrEOF()

0e64d73

dnephin reviewed Sep 18, 2020

View reviewed changes

Pierre Cauchois added 2 commits September 18, 2020 01:16

remove t.Parallel()

aa1875c

ServerError type check before EOF string comparison

352cf93

dnephin reviewed Sep 24, 2020

View reviewed changes

lib/eof.go Outdated Show resolved Hide resolved

use errors.As() for wrapped ServerError

f85fec6

pierreca mentioned this pull request Oct 12, 2020

Enforce bounds on MaxQueryTime hashicorp/nomad#9064

Merged

joel0 mentioned this pull request Oct 15, 2020

Wrap rpc error object #8976

Merged

benbuzbee mentioned this pull request Nov 4, 2020

RPC retry logic in nomad client's rpc.go is incorrect hashicorp/nomad#9265

Closed

dnephin added backport/1.7 type/bug Feature does not function as expected labels Mar 16, 2021

dnephin approved these changes Mar 16, 2021

View reviewed changes

dnephin merged commit 931023f into hashicorp:master Mar 16, 2021

hashicorp-ci pushed a commit that referenced this pull request Mar 16, 2021

Merge pull request #8698 from pierreca/fix-iserreof

fe81353

Use errors.Is() in IsErrEOF()

hashicorp-ci pushed a commit that referenced this pull request Mar 16, 2021

Merge pull request #8698 from pierreca/fix-iserreof

3ade77c

Use errors.Is() in IsErrEOF()

hashicorp-ci pushed a commit that referenced this pull request Mar 16, 2021

Merge pull request #8698 from pierreca/fix-iserreof

a8d48b7

Use errors.Is() in IsErrEOF()

dizzyup pushed a commit that referenced this pull request Apr 21, 2021

Merge pull request #8698 from pierreca/fix-iserreof

f1982a6

Use errors.Is() in IsErrEOF()

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use errors.Is() in IsErrEOF() #8698

Use errors.Is() in IsErrEOF() #8698

pierreca commented Sep 17, 2020

hashicorp-cla commented Sep 17, 2020 •

edited

Loading

dnephin left a comment

dnephin Sep 17, 2020

pierreca Sep 17, 2020

dnephin Sep 17, 2020 •

edited

Loading

pierreca Sep 18, 2020

pierreca Sep 19, 2020 •

edited

Loading

dnephin Sep 17, 2020 •

edited

Loading

dnephin Sep 18, 2020

dnephin Sep 18, 2020

dnephin Sep 18, 2020

dnephin Sep 18, 2020

pierreca commented Sep 21, 2020

dnephin left a comment

pierreca commented Sep 24, 2020

pierreca commented Oct 5, 2020 •

edited

Loading

pierreca commented Oct 14, 2020

dnephin left a comment

dnephin Mar 16, 2021

hashicorp-ci commented Mar 16, 2021

hashicorp-ci commented Mar 16, 2021

hashicorp-ci commented Mar 16, 2021

hashicorp-ci commented Mar 16, 2021

Use errors.Is() in IsErrEOF() #8698

Use errors.Is() in IsErrEOF() #8698

Conversation

pierreca commented Sep 17, 2020

hashicorp-cla commented Sep 17, 2020 • edited Loading

dnephin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dnephin Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierreca Sep 19, 2020 • edited Loading

Choose a reason for hiding this comment

dnephin Sep 17, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pierreca commented Sep 21, 2020

dnephin left a comment

Choose a reason for hiding this comment

pierreca commented Sep 24, 2020

pierreca commented Oct 5, 2020 • edited Loading

pierreca commented Oct 14, 2020

dnephin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hashicorp-ci commented Mar 16, 2021

hashicorp-ci commented Mar 16, 2021

hashicorp-ci commented Mar 16, 2021

hashicorp-ci commented Mar 16, 2021

hashicorp-cla commented Sep 17, 2020 •

edited

Loading

dnephin Sep 17, 2020 •

edited

Loading

pierreca Sep 19, 2020 •

edited

Loading

dnephin Sep 17, 2020 •

edited

Loading

pierreca commented Oct 5, 2020 •

edited

Loading