Refactor VB's Application Framework Network Class

[KlausLoeffelmann]

The class Microsoft.VisualBasic.Devices.Network, which is part of the Visual Basic Application Framework, is based on the obsoleted WebClient class.

We need to modernize this part of the Application Framework to avoid breaking changes, should the WebClient class no longer be included in future versions of .NET.

This should be done in the .NET 9-time frame.

This also addresses https://github.com/dotnet/winforms/issues/3936.

Here is a potential general approach, which uses HttpClient instead of WebClient. (!not tested, and my last Web-dev-experiences is quite a while, so, please double-check me here!)

Imports System.IO
Imports System.Net
Imports System.Net.Http

Public Class Network

    Public Async Function DownloadFileWithCredentialsAndProgressAsAStartingPointAsync(
        url As String,
        destinationPath As String,
        credentials As NetworkCredential,
        progress As IProgress(Of Double)) As Task

        Dim handler As New HttpClientHandler() With
        {
            .Credentials = credentials
        }

        Using httpClient As New HttpClient(handler)

            Dim response = Await httpClient.GetAsync(url, HttpCompletionOption.ResponseHeadersRead)
            Dim contentLength = response.Content.Headers.ContentLength

            Using responseStream As Stream = Await response.Content.ReadAsStreamAsync()
                Using fileStream As New FileStream(destinationPath, FileMode.Create, FileAccess.Write, FileShare.None)

                    Dim buffer(8191) As Byte
                    Dim totalBytesRead As Long = 0
                    Dim bytesRead As Integer

                    While (Await responseStream.ReadAsync(buffer, 0, buffer.Length)) > 0
                        Await fileStream.WriteAsync(buffer, 0, bytesRead)
                        totalBytesRead += bytesRead

                        If contentLength.HasValue Then
                            Dim percentage As Double = (CDbl(totalBytesRead) / CDbl(contentLength.Value)) * 100
                            progress.Report(percentage)
                        End If

                    End While
                End Using
            End Using
        End Using
    End Function
End Class

[ghost]

This issue is now marked as "help wanted", and we’re looking for a community volunteer to work on this issue. If we receive no interest in 180 days, we will close the issue. To learn more about how we handle feature requests, please see our documentation.

Happy Coding!

[elachlan]

My understanding of httpclient is that you should try and reuse a single instance instead of constantly creating new instances.

Should the VB Application framework maintain its own internal instance of httpclient to use throughout? or is this the only usage of it?

[paul1956]

Even if this is the only use of the httpClient, the app might download more than one file in its lifespan.

[elachlan]

You also should not dispose of httpclient. https://medium.com/@nuno.caneco/c-httpclient-should-not-be-disposed-or-should-it-45d2a8f568bc

Edit: clarification, do not use a pattern of create/dispose for each request. Hold an instance of it per thread (its not threadsafe) for the life of the application.

[paul1956]

@KlausLoeffelmann I tried using the code as shown in my application as a test and the .Net Analyzer complains that I should use the Memory Versions vs Byte(), but I am not familiar with Memory(Of Byte) used for read and ReadOnlyMemory(Of Byte) used foy write to convert the code and examples for VB were not very good. If no one else picks this up, I will look into it a little more next week.

[KlausLoeffelmann]

We need to keep in mind not to deviate from the original behavior and consider the special use case here. First, this is purely for downloading a file, so it is an operation which is very likely to be a rather self-contained operation, and HttpClient is not used for other purposes. On top, we can rather expect that, if repeated at all, then downloading is most likely a rather isolated event over the lifespan of an application, so, for that reason I would say, let use and dispose the client as soon the download has finished.

For using Memory<T>: I would cautiously assume the performance difference is negligible, and the actual reason to point that out would be, if that buffer is used repeatedly in an app. Not sure, if that really applies here, for the same reason. But, again without testing, I refactored the code a bit, and this would assumingly work as well:

        Public Async Function DownloadFileWithCredentialsAndProgressAsAStartingPointAsync(
            url As String,
            destinationPath As String,
            credentials As NetworkCredential,
            progress As IProgress(Of Double)) As Task

            Using handler As New HttpClientHandler() With
            {
                .Credentials = credentials
            }

                Using httpClient As New HttpClient(handler)
                    Using response = Await httpClient.GetAsync(url, HttpCompletionOption.ResponseHeadersRead)
                        Using responseStream As Stream = Await response.Content.ReadAsStreamAsync()
                            Using fileStream As New FileStream(destinationPath, FileMode.Create, FileAccess.Write, FileShare.None)

                                Dim buffer(8191) As Byte
                                Dim totalBytesRead As Long = 0
                                Dim bytesRead As Integer

                                While (Await responseStream.ReadAsync(buffer)) > 0
                                    Await fileStream.WriteAsync(buffer.AsMemory(0, bytesRead))
                                    totalBytesRead += bytesRead

                                    Dim contentLength = response.Content.Headers.ContentLength
                                    If Not contentLength.HasValue Then
                                        Continue While
                                    End If

                                    Dim percentage As Double = CDbl(totalBytesRead) / CDbl(contentLength.Value) * 100
                                    progress.Report(percentage)

                                End While
                            End Using
                        End Using
                    End Using
                End Using
            End Using
        End Function
    End Class

But! Same reservations as previously mentioned: My web dev experiences are just not there, so, please double-check me here!

[KlausLoeffelmann]

BTW: For backwards compatibility reasons, we would need the original synchronous version as well.

Like this?

        Public Sub DownloadFileWithCredentialsAndProgressAsAStartingPoint(
            url As String,
            destinationPath As String,
            credentials As NetworkCredential,
            progress As IProgress(Of Double))

            Task.Run(
                Async Function()
                    Await DownloadFileWithCredentialsAndProgressAsAStartingPointAsync(
                        url, destinationPath, credentials, progress)
                End Function).Wait()

        End Sub

[elachlan]

I had no idea the network class existed for downloading files and I have written in vb.net for the last 15+ years. I used webclient for along time and recently I wrote a wrapper for httpclient and used that instead.

I would have expected Application.DownloadFile but not create a Network class instance and call it.

[paul1956]

While this code does not produce VS errors it also doesn't work bytesRead is always 0.

[KlausLoeffelmann]

As I said - never tested it, it is just an idea for an approach. That's why I put it up for grabs! 😸 It's not the highest prio, right now. We just should be prepared for the point in time, should WebClient go away.

[paul1956]

@KlausLoeffelmann I will look at it this/next week. Now sure how I claim it.

[paul1956]

@KlausLoeffelmann I followed all the instructions for installing and starting VS from a script, but I get 91 of the errors below. I do have ,Net 4.8 installed from VS installer but where do I get new compiler?

Severity    Code    Description Project File    Line    Suppression State
Warning CS9057  The analyzer assembly 'C:\Users\PaulM\Source\Repos\winforms\.dotnet\sdk\8.0.100-preview.7.23376.3\Sdks\Microsoft.NET.Sdk\codestyle\cs\Microsoft.CodeAnalysis.CodeStyle.dll' references version '4.8.0.0' of the compiler, which is newer than the currently running version '4.7.0.0'.  Accessibility-version   C:\Users\PaulM\Source\Repos\winforms\.dotnet\sdk\8.0.100-preview.7.23376.3\Sdks\Microsoft.NET.Sdk\codestyle\cs\Microsoft.CodeAnalysis.CodeStyle.dll 1   Active

[elachlan]

That is a nuget package that contains analyzers. It is clashing with the analyzers provided by the .net version. It will be up to the team how they want to fix it, maybe remove the package as it isn't needed anymore?

[paul1956]

@elachlan I need better instruction, what do I remove? Or do I need for an update some someone else?

The Network file includes a lot more than just download as shown. Is the goal to completely remove WebClient and replace it with httpClient.

[elachlan]

The analyzer assembly warning is unrelated to this issue.

It will need to be fixed in another PR.

@lonitra I am unsure who on the team is available to fix it and if we need a separate issue raised?

[lonitra]

@lonitra I am unsure who on the team is available to fix it and if we need a separate issue raised?

Let's raise a separate issue for this so team can triage

[paul1956]

@elachlan @KlausLoeffelmann I made all the required changes in a branch to remove WebClient for Download. Are there any tests for this? Is not is there a public server I can use to test against. I don't have a person web server.

I created Async versions of all 8 download functions, leaving the originals for a total of 16 download functions and just call Async version as appropriate. The one that the work was done in is WebClientCopy is renamed to HttpClientCopy and changes are made to follow the logic in DownloadFileWithCredentialsAndProgressAsAStartingPointAsync above. If this works the same thing can be done to Update, and possibly FTP if needed.

[elachlan]

Awesome! put in a draft PR and we can review it.

Just download this as the test: https://raw.githubusercontent.com/dotnet/winforms/main/README.md

[paul1956]

@elachlan FTP is an issue if anyone cares, the core code is common with Download File. For now I will split it.

https://learn.microsoft.com/en-us/dotnet/core/compatibility/networking/6.0/webrequest-deprecated#recommended-action

[elachlan]

@KlausLoeffelmann the team will need to do an API review for the obsoletion of the ftp functions. You can't avoid it based on the link above. Because FTP is being delegated to third party packages. So its either we mark them as obsolete until runtime removes it, or runtime adds in an ftp client.

[paul1956]

@elachlan I think FTP Upload was deleted but there are remints in VB Newtwork files still around. I don't believe the paths are ever hit but without codeCoverage and tests there is no way for me to be sure.

I build a Scratch WinForms VB project to use for testing, but it will not compile due to the error below I have tried setting 15.0 in the project file as other VB projects do and 16.9 but still get the error.

Severity    Code    Description Project File    Line    Suppression State
Error   BC2014  the value 'preview' is invalid for option 'langversion' ScratchProjectVB    ...\winforms\src\System.Windows.Forms\tests\IntegrationTests\ScratchProjectVB vbc

[paul1956]

@elachlan I pushed what I have to branch FixIssue#9807, the scratch project does not compile so I can't test, and I know the credentials will need to work to use useful. It's been a few years since I contributed to Roslyn, so my Git skills are very poor, let me know if I missed anything.

I did try just opening just the project with my changes and the VB Scratch Project but that make the scratch project worse.

[elachlan]

Looks like you are on the right track. So submit a PR, just navigate to the pull requests tab(on github website) and it should prompt you to submit a PR. In the description mention this issue number.

[paul1956]

@elachlan its untested and I can't test without a VB scratch project, and they don't work.

I did submit a PR in its current state.

[paul1956]

@elachlan I removed scratch project and just created a separate solution about 75% of the new and old download functions work as direct replacement code needed major rewrite and ChatGPT provided the correct algorithms. Everything expect UI work as direct replacement. I haven't started on UI yet that will be most difficult plus. I can't yet figure out how it worked in the old code. I will be running same tests on both.

[paul1956]

@elachlan Ready for review everything is working and tested except things that require Credentials (code is there but I have no way to test) I split HttpClientCopy download from WebClientCopy which handles upload, Upload code is unchanged.