Wrap EOF error instead of replacing it #777
Conversation
Signed-off-by: Josh Humphries <2035234+jhump@users.noreply.github.com>
| // an abnormal end of the operation. | ||
| func wrapIfEOF(err error) error { | ||
| if errors.Is(err, io.EOF) { | ||
| return &unexpectedEOFError{err} |
There was a problem hiding this comment.
I don't think this needs to be a custom error. NewError(CodeUnavailable, err) seems fine.
There was a problem hiding this comment.
No, that doesn't work at all. In fact, that's what the code already does. If you leave that wrapping in place and remove this one, the tests (added in #776) will fail with a mix of "unimplemented" and "unknown" error codes, instead of "unavailable".
I wrote long-ish comments trying to explain why this is needed. Were they unclear?
There was a problem hiding this comment.
Sorry I meant to translate the error but still wrap as a connect-go:
NewError(CodeUnavailable, io.ErrUnexpectedEOF)There was a problem hiding this comment.
Well, that's basically what the other PR did -- it replaced the error instead of wrapping it.
I also now realize that this PR that "wraps" isn't really wrapping it, at least not properly since it doesn't provide an Unwrap() error method. And adding such a method causes the tests to fail... So I'll just stick with what was in #776.
There was a problem hiding this comment.
Sounds good, but for consistency with the other error handlers the wrap was meant purely to wrap it in a connect error code. All the other wrap... functions call NewError so this should wrap in one too. It avoids other wrappers looking at it as each one checks if its already wrapped before processing. It has nothing to do with the Unwrap() error interface (I mean NewError does implement it but thats not the point). My comments were only to enforce the consistency of the current error handling.
This achieves the same result as #776, except it does so by wrapping the error instead of replacing it.
If this looks good, I'll merge it into #776 and then merge that PR to main. If this does not look good, I'll close it without merging and then merge #776 to main as-is.