Revisit IServerFactory.Start

[davidfowl]

Does it really need to pass back in the IFeatureCollection returned in Initialize?

IDisposable Start(IFeatureCollection serverFeatures, Func<IFeatureCollection, Task> application);

Why not just:

IDisposable Start(Func<IFeatureCollection, Task> application);

The current design does allow you to pass in different instances but I'm not sure if that's a goal since we don't use it that way currently.

[Tratcher]

Well as a factory it can be used more than once (not that we actually do this at the moment). Maybe a factory isn't the pattern you want here? Removing that param makes it a statefull single use class. At that point you would just make the factory itself disposable rather than returning anything from start.

[davidfowl]

Thats what I'm wondering. I know the pattern allows for that but I'm not sure we use it or plan to use it anywhere that way. I know this stuff came from katana but I'd like to understand if we ever plan to use it in that way in the current stack.

/cc @loudej @lodejard

[davidfowl]

public interface IServerFactory : IDisposable
{
    IFeatureCollection Initialize(IConfiguration configuration);
    void Start(Func<IFeatureCollection, Task> application);
}

I'm not sure. I guess it does feel a bit off that some of those things are inferred (e.g. the fact that the returned IFeatureCollection should be used later by Start?)

[Tratcher]

How about:

public interface IServer : IDisposable
{
    IFeatureCollection Features { get; }
    void Initialize(IConfiguration configuration);
    void Start(Func<IFeatureCollection, Task> application);
}

Note initialize only exists in this model because we don't have a good way of getting the IConfiguration into the DI constructor.

[davidfowl]

What about:

public interface IServerFactory
{
    IServer CreateServer(IConfiguration configuration);
}

public interface IServer : IDisposable
{
    IFeatureCollection Features { get; }
    void Start(Func<IFeatureCollection, Task> application);
}

[Tratcher]

shrug we can try it. I don't think it solves any issues, it's just API cleanup.

[davidfowl]

This whole issue is about API cleanup. I was demoing the interface recently and decided to file this issue because I thought the stateless nature of the server factory was a little weird.

[Eilon]

After discussion with @lodejard @davidfowl , slight change:

public interface IServerFactory
{
    IServer CreateServer(IConfiguration configuration);
}

public interface IServer : IDisposable
{
    IFeatureCollection Features { get; }
    void Start(RequestDelegate requestDelegate);
}

[Tratcher]

RequestDelegate takes an HttpContext, not an IFeatureCollection. Hosting creates the HttpContext, not the server.

[Eilon]

I think @davidfowl or @lodejard requested that change from IFeatureCollection to HttpContext, but I didn't quite get why.

[davidfowl]

We're unifying the signatures. Everything is HttpContext now and the feature collection becomes and implementation detail of the HttpContextFactory. The layers have been mushed.

[HaoK]

This looks pretty similar to all of the builders we have

Maybe?

public interface IServerBuilder
{
    IServer Build(IConfiguration configuration);
}

[davidfowl]

Except there's nothing to hang off of it.

[HaoK]

So the distinction is that Builder means a fancy Factory that has knobs/settings that you can adjust before calling build?

[davidfowl]

Yah. We aren't actually building/configuring anything just making a sever instance

[JunTaoLuo]

merged to dev

[JunTaoLuo]

--updated from pull request comments

As per emails, the new proposal is as follows:

namespace Microsoft.AspNet.Server.Abstractions
{
    public interface IHttpApplication<THttpContext>
    {
        THttpContext CreateContext(IFeatureCollection contextFeatures);
        Task ProcessRequestAsync(THttpContext httpContext);
        void DisposeContext(THttpContext httpContext, Exception exception);
    }

    public interface IServerFactory
    {
        IServer CreateServer(IConfiguration configuration);
    }

    public interface IServer : IDisposable
    {
        void Start<THttpContext>(IHttpApplication<THttpContext> application);
    }
}

public class HostingApplication : IHttpApplication<HostingApplication.Context>
{
    Context CreateContext(IFeatureCollection contextFeatures) { ... }
    Task ProcessRequestAsync(Context context) { ... }
    void DisposeContext(Context context, Exception exception) { ... }

    public struct Context
    {
        public HttpContext HttpContext { get; set; }
        public IDisposable Scope { get; set; }
        public int TickCount { get; set; }
    }
}

cc @davidfowl @lodejard @Tratcher @troydai @rynowak

[davidfowl]

Updated the spec

[JunTaoLuo]

Updated the spec

[JunTaoLuo]

While looking to adapt the Kestrel server to the new spec, I found that the generic typing is causing us to make many function calls/callbacks to become generic, see aspnet/KestrelHttpServer@3077b96. Maybe we should just use object instead?

cc @halter73 @Tratcher @davidfowl @loudej @rynowak

[halter73]

👍 for using object over trying to flow the generic parameter.

[JunTaoLuo]

Moar updates to the design! Instead of using object, we'll use generics but modify Kestrel's design a bit to make the generic propagation less viral.