use HttpClient to replace WebClient

[kasperk81]

this will unblock https://github.com/dotnet/sdk/pull/41616 see https://github.com/dotnet/runtime/pull/103456

fix https://github.com/dotnet/winforms/issues/11544

Microsoft Reviewers: Open in CodeFlow

[kasperk81]

@akoeplinger @MichalStrehovsky @MihaZupan

[akoeplinger]

this seems like a more involved change, I think I'd prefer suppressing the warning (the code is already doing that in some places) and file a follow-up issue.

[akoeplinger]

There were earlier attempts in https://github.com/dotnet/winforms/pull/2124 and https://github.com/dotnet/winforms/pull/1952, I'd suggest you take a look at the feedback there, especially around making sure there's a shared HttpClient. There's also https://github.com/dotnet/winforms/issues/1756 which I think was inadvertently closed when https://github.com/dotnet/winforms/pull/6684 was merged.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 73.33333% with 4 lines in your changes missing coverage. Please review.

Project coverage is 74.42490%. Comparing base (5e1b5b5) to head (c586555). Report is 3 commits behind head on main.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## main #11542 +/- ## =================================================== - Coverage 74.45532% 74.42490% -0.03043% =================================================== Files 3039 3039 Lines 628902 628889 -13 Branches 46831 46830 -1 =================================================== - Hits 468251 468050 -201 - Misses 157301 157484 +183 - Partials 3350 3355 +5 ``` | [Flag](https://app.codecov.io/gh/dotnet/winforms/pull/11542/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | Coverage Δ | | |---|---|---| | [Debug](https://app.codecov.io/gh/dotnet/winforms/pull/11542/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | `74.42490% <73.33333%> (-0.03043%)` | :arrow_down: | | [integration](https://app.codecov.io/gh/dotnet/winforms/pull/11542/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | `17.90272% <0.00000%> (-0.06986%)` | :arrow_down: | | [production](https://app.codecov.io/gh/dotnet/winforms/pull/11542/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | `47.22898% <73.33333%> (-0.06907%)` | :arrow_down: | | [test](https://app.codecov.io/gh/dotnet/winforms/pull/11542/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | `96.96932% <ø> (+0.00057%)` | :arrow_up: | | [unit](https://app.codecov.io/gh/dotnet/winforms/pull/11542/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet) | `44.29780% <73.33333%> (+0.00764%)` | :arrow_up: | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=dotnet#carryforward-flags-in-the-pull-request-comment) to find out more.

[kasperk81]

@akoeplinger https://github.com/dotnet/winforms/pull/11542/files?w=1

win x64 test is also failing on main https://dev.azure.com/dnceng-public/public/_build/results?buildId=710508&view=logs&j=44ec6876-9b88-5d77-6451-d0463602967d&t=f370c273-05a6-540c-86f1-66d27c79d419

[akoeplinger]

@kasperk81 the winforms maintainers need to ultimately decide but I think we'd get the flow unblocked more easily if we suppress the warning for now.

[elachlan]

@paul1956 I think was working on similar things for Winforms VB.Net.

[paul1956]

I think was working on similar things for Winforms VB.Net.

@elachlan I already did all the work for download in PR #9867 and PR #11215 but they are not being reviewed. Upload is a trivial change but writing a test server for upload is well beyond my ability I wrote all the tests using a public server which I was told was not allowed in Repo. None of the VB Code Upload/Download or FileIO has tests on Main branch, I wrote then to make sure error codes/exceptions are exactly the same. One minor issue is getting the error codes exactly the same requires SR entries and translation that I don't understand.

[lonitra]

@kasperk81 Is this still blocking for https://github.com/dotnet/sdk/pull/41616? This was suppressed in a recent depenedency update above.

[kasperk81]

@lonitra it's now replacing the httpclient to make it future proof. obsoletion is an indication that api may get removed in the future and here it's simply not worth keeping it.

[kasperk81]

Opened https://github.com/dotnet/docs/issues/41485 for docs.

[RussKie]

@lonitra I think this should be filed under "breaking change" with relevant docs, release docs and breaking change docs updated/generated. RE: https://github.com/dotnet/winforms/pull/11542#discussion_r1645701079

[paul1956]

I think this is worse than a breaking change. Current code can distinguish between a wrong password and a offline server or bad URL. After this change everything is an IO Error. This may not apply to loading an image but does apply to replacing WebClient.Sent from my iPhoneI apologize for any typos Siri might have made.(503) 803-6077On Jun 19, 2024, at 2:51 PM, Igor Velikorossov @.***> wrote: @lonitra I think this should be filed under "breaking change" with relevant docs, release docs and breaking change docs updated/generated.

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: @.***>

[kasperk81]

Current code can distinguish between a wrong password

how are you providing password to this internal WebClient today?

After this change everything is an IO Error

HttpClient does not throw I/O exception.

[paul1956]

@kasperk81 my comments refer to the VB network code which currently uses WebClient also and does expose WebClient Exceptions to code calling the network functions. I have not looked at other uses. The comments here seem to say this breaking change is OK but I was hoping for a more holistic solution that covered all WebClient usage.

[lonitra]

Current code can distinguish between a wrong password and a offline server or bad URL. After this change everything is an IO Error.

I'm not sure I follow this. My understanding here is WebClient throws WebException for most of its methods and if we want to find out more about the error, we would need to refer to WebExceptionStatus. HttpRequestException similarly has HttpStatusCode which can be checked to determine more information about the error that occurred. I'm not familiar with VB network code usage of WebClient, but if we had previously handled status codes of the WebExceptions then we should make adjustments to continue to handle them in the same way with HttpRequestExceptions, but in PictureBox case we had not handled it. We have filed breaking change so it is up to caller to handle this for their specific scenario

[paul1956]

@lonitra

All of the VB upload and download functions look like this and they rethrow the WebClient exception. This is from Main

        Public Sub DownloadFile(address As Uri, destinationFileName As String)
            Debug.Assert(m_WebClient IsNot Nothing, "No WebClient")
            Debug.Assert(address IsNot Nothing, "No address")
            Debug.Assert((Not String.IsNullOrWhiteSpace(destinationFileName)) AndAlso Directory.Exists(Path.GetDirectoryName(Path.GetFullPath(destinationFileName))), "Invalid path")

            ' If we have a dialog we need to set up an async download
            If m_ProgressDialog IsNot Nothing Then
                m_WebClient.DownloadFileAsync(address, destinationFileName)
                m_ProgressDialog.ShowProgressDialog() 'returns when the download sequence is over, whether due to success, error, or being canceled
            Else
                m_WebClient.DownloadFile(address, destinationFileName)
            End If

            'Now that we are back on the main thread, throw the exception we encountered if the user didn't cancel.
            If _exceptionEncounteredDuringFileTransfer IsNot Nothing Then
                If m_ProgressDialog Is Nothing OrElse Not m_ProgressDialog.UserCanceledTheDialog Then
                    Throw _exceptionEncounteredDuringFileTransfer
                End If
            End If

        End Sub

Then code like the following works, this is a test I wrote to cover this, but this also exists in applications including one I am maintaining. On bad password it prompts user to reenter and on Timeout it prompts for new download address.

        <WinFormsFact>
        Public Sub DownloadFile_UrlWithTimeOut_Throws()
            Dim testDirectory As String = CreateTempDirectory()
            Dim destinationFileName As String = GetDestinationFileName(testDirectory)
            Dim webListener As New WebListener(DownloadLargeFileSize)
            Dim listener As HttpListener = webListener.ProcessRequests()

            Try
                Dim ex As Exception = Assert.Throws(Of WebException)(  ' <======= this is an issue
                        Sub()
                            My.Computer.Network _
                                .DownloadFile(webListener.Address,
                                              destinationFileName,
                                              userName:="",
                                              password:="",
                                              showUI:=False,
                                              connectionTimeout:=1,
                                              overwrite:=True)

                        End Sub)

                Assert.Equal(SR.net_webstatus_Timeout, ex.Message) ' <======= this is an issue
                Assert.False(File.Exists(destinationFileName))
            Finally
                CleanUp(listener, testDirectory)
            End Try
        End Sub

Separate issue SR.net_webstatus_Timeout and SR.net_webstatus_Unauthorized are not accessible to the VB code in WinForms Repo.

[elachlan]

@paul1956 I think because its internal here its not so much an issue. VB.net will be different.

[lonitra]

@paul1956 Let's chat about VB scenario in your PR.