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?