microsoftgraph/msgraph-sdk-dotnet-core

Too much TargetFrameworks, and HttpClientResponsibility

svrooij opened this issue · 8 comments

This is not so much an issue, more a design decision.

Currently the TargetFrameworks are really big, like <TargetFrameworks>netstandard2.0;net462;net6.0;net6.0-android;net6.0-ios;net6.0-maccatalyst;net6.0-macos;net6.0-windows</TargetFrameworks>

To my understanding this is purely so it can create an efficient HttpClient for each platform. That seems to be the only reason why it's configured like this. So my question is:

Why is it this library' responsibility to create an efficient HttpClient? And shouldn't you guys just stimulate the use of Microsoft.Extensions.Http for generating the most efficient httpclienthandler and keeping track of it so it does not run out of available sockets.

Reasons I ask:

  • Current implementation fails in Blazor Webassembly, (but is worked around with this fix, by using Microsoft.Extensions.Http anyways)
  • Having such a big TargetFrameworks, makes contributing immensely difficult, since you would need to have all those frameworks installed to be able to contribute, else the project will not build and you get all those red lines in VS.

I would opt for a solution that accepts an HttpClient and sets the default with new HttpClient() if not specified, no trickery with for all the different platforms.

To further iterate the http issue, because this library doesn't make use of Microsoft.Extensions.Http, sockets are not managed according to best practices and the performance is subpar compared to IHttpClientFactory-backed http clients.

@MIchaelMainer thoughts? We're starting to see challenges across multiple applications along these lines. It seems the proposal in this issue could address the concern without a breaking change. Should we just submit a PR?

@adamedx while my proposed solution is not a breaking change, it still is a big change. Because the current solution tries do do some efficiently steps. All those would be removed if you remove all the extra targeted frameworks.

I would still like to have the development team consider this.

  1. Enabling easier contributions
  2. Not managing getting an efficient HttpClient in every other library, and delegating that responsibility to a single package (and/or the developer).
  3. Less magic behind the scenes
  4. Way faster build times
  5. Way smaller package on nuget because of less target frameworks

Partially related to #510. If I understand correctly, IHttpClientFactory handles getting the appropriate client as it does the conditional checks itself for the target frameworks so that in this library, we wouldn't need the #if preprocessor directives to get the correctly configured HttpClient for the target framework.

https://github.com/microsoft/kiota-http-dotnet would need to implement Microsoft.Extensions.Http.

I defer to @andrueastman who owns this project on how to move forward.

We are trying to add some better transient fault handling since the built-in RetryHandler only handles rate limiting and gateway timeouts. We want to add TCP connection timeout retries, etc.

IHttpClientFactory would allow us to hook Polly retry policies up in a straightforward way. Polly's APIs are targeted at the DI service registration and it is not obvious how to hook them up to the HttpClient returned by GraphClientFactory.Create(...)

This seems to be blocking adoption @MIchaelMainer and may also be a COGS issue. Is there a way we can prioritize a decision on this in some time frame so we can get people out of a holding pattern? We really don't want people rolling their own libraries!