use HttpClient to replace WebClient

[kasperk81]

this will unblock dotnet/sdk#41616
see dotnet/runtime#103456

fix #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 #2124 and #1952, I'd suggest you take a look at the feedback there, especially around making sure there's a shared HttpClient.
There's also #1756 which I think was inadvertently closed when #6684 was merged.

[codecov]

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

@@                 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	Coverage Δ
Debug	74.42490% <73.33333%> (-0.03043%)	⬇️
integration	17.90272% <0.00000%> (-0.06986%)	⬇️
production	47.22898% <73.33333%> (-0.06907%)	⬇️
test	96.96932% <ø> (+0.00057%)	⬆️
unit	44.29780% <73.33333%> (+0.00764%)	⬆️

Flags with carried forward coverage won't be shown. Click here 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 dotnet/sdk#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.

[lonitra]

Changes LGTM, thank you! 🥳 We'll still need to do some slight documentation adjustments to reflect WebClient is no longer being used

[kasperk81]

Opened dotnet/docs#41485 for docs.

[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.