Skip to content

Pass underlying network error in ErrorConnectionFailed#41292

Merged
thaJeztah merged 1 commit intomoby:masterfrom
eelf:pass_network_error
Aug 4, 2020
Merged

Pass underlying network error in ErrorConnectionFailed#41292
thaJeztah merged 1 commit intomoby:masterfrom
eelf:pass_network_error

Conversation

@eelf
Copy link
Copy Markdown
Contributor

@eelf eelf commented Jul 29, 2020

- What I did
I changed the check for context error returned by net/http.Client.Do
- How I did it
by using github.com/pkg/errors.Is that searches for desired error in err's chain
- How to verify it
I've encountered shallow error message telling me to check if docker daemon is running but actual problem was in context deadline exceeding. To verify pass to client a context with a deadline that is in the past.
- Description for the changelog
Fixes context errors being treated as connectivity error in client

- A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: Evgeniy Makhrov e.makhrov@corp.badoo.com

Comment on lines 151 to 157
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm a bit concerned about adding the raw error after already trying to check what's the cause in this block.

You mentioned you had a deadline exceeded; wouldn't that already be return the raw error (handled higher up in this file)?

moby/client/request.go

Lines 135 to 140 in 663d143

// Don't decorate context sentinel errors; users may be comparing to
// them directly.
switch err {
case context.Canceled, context.DeadlineExceeded:
return serverResp, err
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

net/http.Client.Do returns url.Error with Err error field being set to context.DeadlineExceeded error, so it passes through this switch
Maybe it's needed to try to Unwrap it first

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, if it doesn't catch that, that may be a bug

Signed-off-by: Evgeniy Makhrov <e.makhrov@corp.badoo.com>
@eelf eelf force-pushed the pass_network_error branch from 6f53815 to 8ccb46a Compare August 3, 2020 13:00
@eelf
Copy link
Copy Markdown
Contributor Author

eelf commented Aug 3, 2020

@thaJeztah could you please have a look. I've force-pushed to same PR and updated description.

Copy link
Copy Markdown
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, did this solve the problem in your situation?

@eelf
Copy link
Copy Markdown
Contributor Author

eelf commented Aug 3, 2020

LGTM, did this solve the problem in your situation?

Yes, I've checked with context having enough timeout and exceeded deadline. It now shows me nice error that communication with docker.sock failed due to "context deadline exceeded" error

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants