search

ramondeklein / nwebdav

.NET implementation of the WebDAV protocol
MIT License
155 stars 42 forks source link

Upgrade to .NET5 #60

Closed MichaelPeter closed 3 years ago

MichaelPeter commented 3 years ago

TODO: Commits need to be squashed. TODO2: Remove support for netstandard 1.6? TODO3: Add Support for additional Method TODO4: Kick the butt of IT-Hit for charging so high prices for WebDav.

ramondklein: Contact me if you are still interested in the project. Have some ideas, also regarding Sharepoint specifics michaelpeteronline@googlemail.com

MichaelPeter commented 3 years ago

Not sure if I made the changes in the way you would have wanted them to be made. Feedback highly appriciated.

MichaelPeter commented 3 years ago

Question here is if IAsyncCollection makes sense. It would only be advantagous for scenarios with big folders (5.000+ Items) where the http messages is streamed back to the client. And the client starts displaying the contents of a folder while they arrive in the http stream.

Against it speaks if maybe IAsyncCollection maybe has some performance pentality. The unterlying implementation does not yet take advantage of the IAsync enumerable.

ramondeklein commented 3 years ago

Thanks for your input and contribution. I am a bit busy at this moment, but I'll try to review your changes this week. I would like to keep compatibility with the traditional .NET Framework for now, unless there are important reasons (this project is often used in .NET Framework v4.x projects). Using IAsyncEnumerable would only be useful when you need to access external resources that return information in chunks. In most situations, it won't have any benefits and it's not trivial to support it in classic .NET Framework.

MichaelPeter commented 3 years ago

Hello thanks for your feedback.

When you review the changes there are Precompiler conditions the IAsyncEnumerable usage in the code is limited for Net Standard 2.1 and Net5.0 and up. All previous versions Net 4.X or NetStandard 2.0 or lower which do not support IAsyncEnumerable yet, still have an interface Task<IEnumerable>.

What breaking change I introduced for .NET 4.X, GetItemsAsyncreturns now Task<IEnumerable<IStoreItem>> instead of Task<IList<IStoreItem>> since the advantages of IList (Like Count) have not been used. This way the resultset COULD also be streamed. But this breaking change means you just have to change a return type.

NetCore/Net 5 will have the breaking change of now returning IAsyncEnumerable.

The question is if any webdav client takes advantage of streaming folder content. Since WebDav does not support anything like paging.

The suggestion for removal of support for Net Standard 1.6 was because only NetCore 1.0 requires it. And the only scenario I know where Net Standard 2.0 is required is UWP Apps on Xamarin. And you wouldn't host a WebDav Server in UWP. And I think most developers switched from NetCore 2.0 to NetCore 2.1 on the server side.

Here Net Standard support: https://docs.microsoft.com/en-us/dotnet/standard/net-standard

MichaelPeter commented 3 years ago

Also a small hint regarding the switch from XDocument.Load(request.Stream) to XDocument.LoadAsync(request.Stream).

Starting from Aspnetcore 3.1 Syncronous Stream reads are by default disabled (AllowSynchronousIO https://github.com/dotnet/aspnetcore/issues/7644) So with default settings XDocument.Load will throw a Exception if AllowSynchronousIO is not explitictily set. But AllowSynchronousIO is only a compatibility switch.

Now XDocument.LoadAsync is only available since Net Standard 2.1, so previous versions cannot use it.

tdhintz commented 3 years ago

I can't yet switch to .Net Core 5 so I appreciate the continued compatibility.

ramondeklein commented 3 years ago

I reviewed your changes and I have several comments:

  1. The CSPROJ files are overhauled, so it's difficult to see what has actually been changed. It adds <ApplicationIcon/>, <StartupObject/>, ... which are pretty pointless.
  2. Why upgrade the Kestrel sample to .NET 5? It doesn't bring any improvements and only reduces compatibility.
  3. Why add the net5.0 target for the NwebDav.Server and NWebDav.Server.AspNetCore projects? Using netstandard2.1 brings you all the features you need. This is only addititonal work.
  4. Using an async foreach would only be benificial when you have a lot of items that need to be fetched asynchronously on a high-concurrent server. To maintain backwards compatibility with older versions, you need a lot of additional code that is hard to maintain.

I do think there is a lot to improve on NWebDav, but just aiming to use the latest .NET 5 features is not my goal. I would rather improve the logging framework (just inject ILogger) and the ability to use the standard .NET DI container. But I don't want to make much breaking changes. NWebDav is used in several projects and not everybody can switch to the latest and greatest .NET version. I want to maintain compatibility with older frameworks as longs as they are still relevant.

MichaelPeter commented 3 years ago

Hello @ramondeklein,

  1. I removed ApplicationIcon and StartupObject, I did not add them intentionally, seems to have been a some visual studio functionality has been fixed.

  2. I must admit I was not sure what is still the purpose of the kestrel sample since it was conceived in the infancy of netcore. To me it seemed like a sample for what is now aspnetcore. And samples are mostly for new applications, which tend to be on newer frameworks.

  3. I agree .Net Standard 2.1 would be sufficient. I added a .NET5 since Microsoft announced that .NET5 should replace .NET Standard. But for now it could be removed since it bears no advantage..

See here: https://visualstudiomagazine.com/articles/2020/09/16/net-standard-future.aspx https://devblogs.microsoft.com/dotnet/the-future-of-net-standard/

Think it is also a bit of a philosophical question. Do you go for the new an shiny or the old established that actually makes money. Think you need both, backwards compatibility and the new and shiny, depending what you want. And you shouldn't just do what Microsoft tells you to do.

But we arrived in a new nodejs and javascript world, where Long Term Support .NET Versions means 3 Years support from Microsoft and where NetCore 1.X is obsolete like .NET Framework 1.0 when .NET Framework 2.0. became the Minimum supported Framework Version.

.NET Framework is getting retried / will very slowly die. We can like it or not, but it is a fact. Is NWebDav a framework that preserves or that moves forward? So far it can still be both.

But yea Net Standard 2.1 would be sufficent for now.

  1. For me either way is ok. But would you stay with Task oder would you switch to Task, latter would break compatibility.

  2. Here I agree with the ILogger topic but for me it was importaint to get NWebDav running without having to activate the compatibilty switches in NET5 (AllowSynchronousIO)

ramondeklein commented 3 years ago

I agree with your second point that Kestrel based applications will typically be .NET Core and I don't think a lot of people are still targetting pre .NET Core 3, so no problem there.

Although .NET Standard 1.6 isn't used a lot, there is no need to upgrade to .NET Standard 2.0, because I don't think we need any features of .NET Standard 2.0. Switching to .NET Standard 2.1 is only suitable when you have lots of items in your collections and you are able to asynchronously provide them using IAsyncEnumerable, which is an edge-case for now (might change in the future when IAsyncEnumerable is more widely adopted).

Supporting older frameworks is not a philosophic question to me, but there are a lot of legacy applications that are written in .NET Framework and won't be ported to .NET Core soon (or maybe never). I am working for two companies that both have a large .NET Framework codebase that won't be ported. One of them also uses NWebDav (that's why I created it in the first place). Some code is even running .NET Framework v4.6, because it runs on old systems and isn't even capable of running .NET Standard 2.0. We are slowly migrating to .NET Framework v4.8, but even this is a precarious process when you have to deal with clients that run older Windows versions. That's why compatibility is a huge deal to me.

If the IAsyncEnumerable implementation would only be visible within a single implementation, then an #if would have been fine, but if you have different interfaces on different platforms, then it becomes messy and requires a lot of code changes. It feels like it is used, because we can. Not because we need to. Hope this clarifies my objections to the IAsyncEnumerable implementation.

Supporting ILogger and being able to plug in to the current .NET Core DI framework implementation is a change that is on my agenda for months, but I am currently working 50hrs+ on paid projects, so this project doesn't get the required attention at this time :-( I also don't want to break .NET Framework v4.5+ compatibility there, so it should be possible to use NWebDav without the .NET Core DI container and inject the classes manually (which should be the goal for each DI app anyway).

ramondeklein commented 3 years ago

I do think we should get rid of AllowSynchronousIO in .NET 5. What's the required change to do that?

MichaelPeter commented 3 years ago

As written with this push Request the need for AllowSycronousIO has been solved, the Problem is starting with AspNetCore 3.1 you you cannot make sync stream reads anymore.

The problem was that XDocument.Load reads the stream sync. The new XDocument.LoadAsync fixes that but XDocument.LoadAsync is only available starting from Net Standard 2.1 and Net 5.

So regarding the Framework: Target Frameworks would then be Net452?, Net Standard 1.6 (as you wrote no difference to Net Standard 2.0), Net Standard 2.1 (And maybe net5.0, even thought I also don't know an advantage yet.)

Regarding IAsyncEnumerable I agree. Maybe there could be an extension Interface, which allows an alternative implemenation, which is called if it is there. I also wanted to test if there are performance pentalities for IAsyncEnumerable.

And no I don't want to break compatibility with Net Framework. But NetCore and Net5+ has differenet demands than Net Framework. And .Net Framework could stay compatible but for newer .NetCore and NET5+ the new capabilities could be supported.

And yes the Code gets with preprocessor commands more messy, but if you want to keep the same codebase for both, I think in future there is no other way (already XDocument.Load and XDocument.LoadAsync demand that)

There are also capabilities which could even be advantagous for Net Framework 4.X, like nullable types, which does not break compatibilitiy.

MichaelPeter commented 3 years ago

My demand is I need this library for my work on a new .NET5, maybe soon even .NET6 Project So as you need it for Net 4.8, I need it for Net5 ;)

MichaelPeter commented 3 years ago

I switched back from IAsyncEnumerable to Task<IEnumerable> since there is no advantage in IAsyncEnumerable right now. Maybe later another interface could be defined that supports this.

Btw: I also checked out the Performance of IAsyncEnumerable and it is actually extremly fast: In 1 Millisecond it can iterate 10000 times, in 1 second 10 million times on a single core: https://github.com/MichaelPeter/TestAsyncEnumerablePerformance/blob/master/TestAsyncEnumerablePerformance/Program.cs

Also I removed the target .Net5 and left just NetStandard 2.1, since Net5 has no advantage yet.

Also bumped the version from 1.34 to 2.01, since there are major changes, including the switch to IEnumerable

ramondeklein commented 3 years ago

@MichaelPeter I have made some additions to your fork. Can you grant me access so I can push them to your repo? Don't bother... I was using the wrong account :-)

ramondeklein commented 3 years ago

@MichaelPeter Just some remarks on my changes:

  1. I think the overhead of an async method (that is actualy sync on <.NET standard 2.1) outweighs the added complexity in all handlers. (commit fe9fac015fab2aff64d015b672a980e5401de72e).
  2. The HTTP listener shouldn't be used on .NET core systems, so I reverted it back to it's original state and disabled .NET core functionality. The largest drawback of HTTP listener is that you cannot cancel requests (Kestrel has a RequestAborted cancellation token). Next step is to properly implement this for all handlers (HTTP listener could just pass CancellationToken.None in that case).
  3. Some small code changes that I prefer :-)
MichaelPeter commented 3 years ago

Hello @ramondeklein,

regarding your points - works for me!

  1. everybody has their own preferences and thats fine :)

So next step would be a new release with a updated nuget package?

Greetings Michael