Skip to content
This repository has been archived by the owner on Dec 19, 2018. It is now read-only.

Hosting API followup #530

Closed
JunTaoLuo opened this issue Dec 21, 2015 · 7 comments
Closed

Hosting API followup #530

JunTaoLuo opened this issue Dec 21, 2015 · 7 comments

Comments

@JunTaoLuo
Copy link
Contributor

Continuing discussion on items left over from #525

  1. Configuration
    Currently we use recommend using builder.UseConfiguration(WebApplicationConfiguration.GetDefault(args)). This is verbose. One suggestion for this is to add a UseConfiguration(string[] args) overload for only command line configs. Also, there is some contention on whether environment variables and hosting.json configurations should be enabled by default.
  2. WebApplication void Start() vs IDisposable Start()
    There are two suggestions for WebApplication.Start(). We currently use the IDisposable pattern which lends itself to a convenient pattern using(app.Start()){ ... } where we handle shutdown through Dispose(). However, since we do not support an application being started more than once, it is natural to return void. This leads to less desirable usage patterns however, such as using(var app = builder.Build()){app.Start(); ... }.
  3. WebApplicationBuilder.Run()/.Start()
    Currently we build an application using a builder, configure that application (such as setting addresses), then run or start the application. There is a suggestion on making address configuration more first class on the WebApplicationBuilder and directly calling Start() or Run() on the builder but this will require shuffling of startup since Addresses are currently ServerFeatures that are only available after the WebApplication is built. It's also suggested to possibly add overloads to Start() and Run() to take in a list of addresses to listen on.
  4. Exception messages
    Some of the current exception messages need to be updated. For example, the message at StartupLoader.cs#L46 can be cryptic. For this exception in particular, it should be determined if it's the responsibility of the WebApplication or the StartupLoader, which could be third party, to throw exception to the user.
@davidfowl
Copy link
Member

Also, there is some contention on whether environment variables and hosting.json configurations should be enabled by default.

Since we have the helper that creates the right IConfiguration with the default behavior, this could go either way. It's easy to turn it on or off by calling UseConfiguration.

builder.UseConfiguration(WebApplicationConfiguration.GetDefault(args))

It is too verbose, I do worry about having UseConfiguration(args) because it's not useful outside of configuring command line args nor does it compose with the other UseConfiguration overload. When we're about to make decisions on these APIs we should have a few scenarios in mind:

  1. Give me the defaults
  2. I want the default and I want to override 1 configuration option thing
  3. Give me something without defaults

Once we cover those 3 we're good.

However, since we do not support an application being started more than once, it is natural to return void

That's a valid reason to return void. Today it does look like you can restart the server by calling start again after dispose. We should have a sample showing how to restart a server, that's something we didn't have guidance on in the past and should be solved with this API do-over. I think this API this ugly:

var application = new WebApplicationBuilder()
                        .UseConfiguration(WebApplicationConfiguration.GetDefault(args))
                        .UseStartup<Startup>()
                        .Build();

            using (application)
            {
                application.Start();
                Console.ReadLine();
            }

There is a suggestion on making address configuration more first class on the WebApplicationBuilder and directly calling Start() or Run() on the builder

Please not Start/Run on the builder that doesn't seem right. Passing the list of addresses to Start/Run seems cleaner but then we might end up in overload hell again when we figure out we want to pass more things to the server (random other server config?).

@JunTaoLuo
Copy link
Contributor Author

            using (application)
            {
                application.Start();
                Console.ReadLine();
            }

Not only is this ugly, it's also not recommended since application is still in scope after the closing brace of the using statement. User can still call member functions on application which, since Dispose() has been called, can be unpredictable.

@davidfowl
Copy link
Member

-2 points for that pattern. We should work on the restart pattern and the pattern that throws if you start more than once. That's a good compromise.

@Tratcher
Copy link
Member

Restart? The current restart pattern is to throw everything away and start from scratch. What would you want to re-use?

@davidfowl
Copy link
Member

Well is it? Even the builder? We should write a few samples showing that.

@JunTaoLuo
Copy link
Contributor Author

Is there any more to this issue after #553?

@JunTaoLuo
Copy link
Contributor Author

Seems like there isn't any more to do in this issue, closing for now.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants