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

Add NetworkError.TimedOut, rename Unknown to Other #40841

Merged
merged 5 commits into from
Aug 17, 2020
Merged

Add NetworkError.TimedOut, rename Unknown to Other #40841

merged 5 commits into from
Aug 17, 2020

Conversation

antonfirsov
Copy link
Member

  • Adds NetworkError.TimedOut
  • Renames Unknown to Other assuming my proposal gets accepted, will revert if not
  • Improves exception message

Will add a case for TimedOut as soon as the Connection PR gets merged. (Based on local test it gets translated fine.)

@Dotnet-GitSync-Bot
Copy link
Collaborator

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@ghost
Copy link

ghost commented Aug 14, 2020

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@antonfirsov antonfirsov added this to the 5.0.0 milestone Aug 14, 2020
@antonfirsov
Copy link
Member Author

/cc @scalablecory @geoffkizer

@jaredpar jaredpar mentioned this pull request Aug 14, 2020
Closed
@geoffkizer
Copy link
Contributor

@antonfirsov The failure in System.Net.Requests.Tests looks related, though I'm not sure how your change would affect this... can you take a look?

@geoffkizer
Copy link
Contributor

That also could have been caused by the recent ConnectionFactory PR.

@ManickaP
Copy link
Member

ManickaP commented Aug 15, 2020 •
edited
Loading

The problem is in missing mapping in https://github.com/dotnet/runtime/blob/master/src/libraries/System.Net.Requests/src/System/Net/WebException.cs#L74

@antonfirsov if you're afk, I can try to fix it.

I think this might be fixed by #40880. Since we have NetworkException bubbling out of HttpClient here and we should be mapping it into HttpRequestException, which already is properly handled by WebException mapping.

@ManickaP

This comment has been minimized.

Sign in to view
@azure-pipelines

This comment has been minimized.

Sign in to view
@ManickaP
Copy link
Member

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@antonfirsov
Copy link
Member Author

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@antonfirsov
Copy link
Member Author

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@antonfirsov antonfirsov mentioned this pull request Aug 15, 2020
Closed
@antonfirsov
Copy link
Member Author

/azp run runtime

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@antonfirsov
Copy link
Member Author

/azp run runtime-libraries outerloop

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

This was referenced Aug 17, 2020
Closed
Closed
@antonfirsov
Copy link
Member Author

OuterLoop test failures are unrelated. Opened new issues: #40926, #40927

@antonfirsov antonfirsov merged commit 296c90a into dotnet:master Aug 17, 2020
@runfoapp runfoapp bot mentioned this pull request Sep 29, 2020
Closed
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@geoffkizer geoffkizer geoffkizer approved these changes

@scalablecory scalablecory scalablecory approved these changes

@stephentoub stephentoub stephentoub approved these changes

Assignees
No one assigned
Labels
area-System.Net new-api-needs-documentation
Projects
None yet
Milestone
5.0.0
Development

Successfully merging this pull request may close these issues.
6 participants
@antonfirsov @Dotnet-GitSync-Bot @geoffkizer @ManickaP @stephentoub @scalablecory