Change DispatchInterceptor API

[bernardnormier]

This PR changes the DispatchInterceptor API to be more like ASP.NET's request delegate API.

See https://www.stevejgordon.co.uk/how-is-the-asp-net-core-middleware-pipeline-built

Just like in ASP.NET, a dispatch interceptor is now a Func<Dispatcher, Dispatcher> while a "simple" dispatch interceptor (with a parameterless next) has a more complicated Func definition.

[bentoi]

The signature of this delegate is a bit difficult to understand. We could consider adding delegate definitions instead:

delegate ValueTask<OutgoingResponseFrame> NextDispatcher();

delegate ValueTask<OutgoingResponseFrame> SimpleDispatcher(IncomingRequestFrame frame, Current current, NextDispatcher next, CancellationToken cancel);

Or perhaps just define NextDispatcher and use Func<IncomingRequestFrame, Current, NextDispatcher, CancellationToken, ValueTask<OutgoingResponseFrame>> in the Use signature.

[pepone]

Agree will be good to simplify this signature

[bernardnormier]

Here again it's like ASP.NET, where they don't define a delegate for the simple middleware registration:
https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.builder.useextensions.use?view=aspnetcore-5.0

[pepone]

Looks good!

[pepone]

using System.Collections.Immutable; is no longer required in this test

[externl]

Looks good. I find "simple" and "regular" somewhat confusing.

[externl]

• Is the user ever expected to use the non-so-simple ObjectAdapter.Use directly or only ever use this extension method?

• Why does this need to be an extension and not just defined in the ObejctAdapter class directly?

• We should clarify this comment.

This is an adapter for <see...

[bentoi]

My understanding is that extensions are supposed to be used to extend classes from some dependant assembly (e.g.: TaskExtensions). So here this method could indeed directly go on the adapter class instead. It will be easier to find when browsing the documentation.

[bernardnormier]

My understanding is that extensions are supposed to be used to extend classes from some dependant assembly (e.g.: TaskExtensions). So here this method could indeed directly go on the adapter class instead. It will be easier to find when browsing the documentation.

I don't believe this is correct.

Extension methods (and functions found through ADL in C++) are preferred for code that does not need private / internal access to the class implementation. It doesn't matter where it is.

In this particular Use case, it's just like ASP.NET and its UseExtensions class that provides a simple "middleware" registration:
https://docs.microsoft.com/en-us/dotnet/api/microsoft.aspnetcore.builder.useextensions.use?view=aspnetcore-5.0

Note that both UseExtensions and IApplicationBuilder are in the same assembly (Microsoft.AspNetCore.Http.Abstractions.dll).

[bentoi]

Yes, looks like ASP.NET loves extensions :). From the Socket API guys, it looks like they'd rather get rid of extensions according to this bug dotnet/runtime#33417

I find it simpler to not have extensions but if it's not recommended... ok.

[externl]

I also think it's simpler to not have extensions.

[pepone]

the fact that this is implemented as an extension ensures this is not coupled to the private implementation as extensions are using the public API, here it can use internals because it is in the same assembly.

[bentoi]

I don't see why we need to expose this non-coupling detail to the user. He doesn't care and I don't think we should care that much about it, at least not to the point of spreading the API in many extensions like ASP.NET does.

[bernardnormier]

I like extensions and also "free" C++ functions found through ADL. However, we don't need an extension class per method like ASP.NET ... I agree that's overkill.