New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update libuv references in Kestrel docs #6184

Merged
merged 12 commits into from May 10, 2018

Conversation

Projects
None yet
5 participants
@guardrex
Collaborator

guardrex commented May 2, 2018

Fixes #5695

@guardrex guardrex requested review from halter73, Tratcher and natemcmaster May 2, 2018

@natemcmaster

A few points to clarify. Thanks for getting started on this.

With the release of ASP.NET Core 2.1, Kestrel's default transport is no longer based on Libuv but instead based on managed sockets. This is a breaking change for ASP.NET Core 2.0 apps that call [WebHostBuilderLibuvExtensions.UseLibuv](/dotnet/api/microsoft.aspnetcore.hosting.webhostbuilderlibuvextensions.uselibuv) and depend on either of the following packages:
* [Microsoft.AspNetCore.Server.Kestrel](https://www.nuget.org/packages/Microsoft.AspNetCore.Server.Kestrel/)
* [Microsoft.AspNetCore.App](https://www.nuget.org/packages/Microsoft.AspNetCore.App/)

This comment has been minimized.

@natemcmaster

natemcmaster May 2, 2018

Member

Since Microsoft.AspNetCore.App is new in 2.1, I might change the phrasing. This is only a breaking change for those using the Microsoft.AspNetCore.Server.Kestrel package directly without references to any metapackages.

Users upgrading from 2.0 to 2.1 shouldn't break unless they also take action to move from .All to .App. But in that case, they will break in a bunch of other ways too since .App doesn't have Redis, Sqlite, etc.

This comment was marked as resolved.

@natemcmaster

natemcmaster May 2, 2018

Member

Oops, I meant .App doesn't have Redis, Sqlite.

```
> [!NOTE]
> The [Microsoft.AspNetCore.All](xref:fundamentals/metapackage) metapackage doesn't directly reference [Libuv](https://www.nuget.org/packages/Libuv/), but it does directly reference [Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv](https://www.nuget.org/packages/Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv/). The Libuv transport package is transitively referenced when the `Microsoft.AspNetCore.All` package is referenced. No action is required by the developer when the `Microsoft.AspNetCore.All` metapackage is used and `UseLibuv` is called in the app. Note that the ASP.NET Core 2.1 or later templates reference the [Microsoft.AspNetCore.App](https://www.nuget.org/packages/Microsoft.AspNetCore.App/) metapackage by default. The use of the `Microsoft.AspNetCore.All` metapackage isn't recommended for ASP.NET Core 2.1 or later apps.

This comment has been minimized.

@natemcmaster

natemcmaster May 2, 2018

Member

This is pretty wordy. I think your main point here is that the previous step is unnecessary in some cases, right? If so, consider shortening to

No additional reference is required when using the Microsoft.AspNetCore.All metapackage, which will pull in Libuv transitively. For more details on this metapackage, see {cref}

@guardrex

This comment has been minimized.

Collaborator

guardrex commented May 2, 2018

@natemcmaster ok ... see how that shorter version looks.

@@ -75,6 +82,24 @@ ASP.NET Core project templates use Kestrel by default. In *Program.cs*, the temp
[!code-csharp[](kestrel/samples/2.x/Program.cs?name=snippet_DefaultBuilder&highlight=7)]
::: moniker range=">= aspnetcore-2.1"
**Default transport changes from Libuv to managed sockets (ASP.NET Core 2.1 or later)**

This comment has been minimized.

@natemcmaster

natemcmaster May 2, 2018

Member

Reviewing this in context of the rest of the document now...

I think this new block of text belongs lower in the doc. Transport configuration deserves its own section, maybe after "Endpoint configuration" and before "URL prefixes". The breaking change from Libuv to Sockets should be one of the things mentioned in that section, but I don't expect this breaking change to affect many people. We're hoping that in most cases no action will be required.

(I'm not expecting you to write this, but) it would also be good to have the transport configuration section explain why you might want to use sockets vs libuv. That can be punted for now, but it's worth addressing because changing the transport off default isn't a common scenario. We should be able to justify why it's ever needed.

@guardrex

This comment has been minimized.

Collaborator

guardrex commented May 3, 2018

@natemcmaster Sounds good. I moved the section down on the last commit.

it would also be good to have the transport configuration section explain why you might want to use sockets vs libuv

Can you or one of the engineers provide the content? ... even if only a list of pros/cons of each or a single list of why sockets are better. It would be good for us to get enough coverage in place for now to close this issue and not open a new one (if possible).

@guardrex

This comment has been minimized.

Collaborator

guardrex commented May 3, 2018

Are the motivations for sockets over Libuv you laid out in aspnet/KestrelHttpServer#2360 (comment) applicable for the community?

Motivations:

  • broad platform support. Wherever managed sockets are supported, Kestrel should work. Doesn't require recompiling libuv to bring up a new OS.
  • source-build. (https://github.com/dotnet/source-build). In current form, Kestrel packages can't be produced in a source-build environment
  • ecosystem. Helps to drive improvement to the .NET sockets implementation, which benefits anyone else using managed sockets.

If so, I can draft some text that we can work forward.

@halter73

This comment has been minimized.

Member

halter73 commented May 3, 2018

@guardrex Those are the primary motivations to default to the socket transport. I don't think we should call too much attention to configuring Kestrel transports in the docs though. I expect that the difference between transports won't be noticeable to the vast majority of developers.

Version="2.1.0" />
```
No action is required when the [Microsoft.AspNetCore.All](xref:fundamentals/metapackage) metapackage is used and `UseLibuv` is called because the Libuv transport package is transitively referenced by the `Microsoft.AspNetCore.All` metapackage. Note that the ASP.NET Core 2.1 or later templates reference the [Microsoft.AspNetCore.App](https://www.nuget.org/packages/Microsoft.AspNetCore.App/) metapackage by default. The use of the `Microsoft.AspNetCore.All` metapackage isn't recommended for ASP.NET Core 2.1 or later apps.

This comment has been minimized.

@halter73

halter73 May 3, 2018

Member

I didn't realize that Microsoft.AspNetCore.All was deprecated. Maybe we shouldn't mention it at all and just tell people to reference Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv directly.

This comment has been minimized.

@guardrex

guardrex May 3, 2018

Collaborator

I believe they're phrasing it more along the lines of "not recommended" and "subject to future removal" at this time. That's why I slanted the language in this way (i.e., not use the word "depreciated" there). *they = @DamianEdwards in Standup remarks.

This comment has been minimized.

@natemcmaster

natemcmaster May 3, 2018

Member

This is a good point. I'm not sure this is a recommendation we actually need to make yet. But even if we do, this seems like information that belongs on the metapackage doc, not here.

@natemcmaster

This comment has been minimized.

Member

natemcmaster commented May 3, 2018

I would leave out the reasons for changing the default. IMO the only important reason document transport config at all is for compatibility. If users upgrade to 2.1 but rely on some libuv-only features, they will need to switch their transport to Libuv. At the moment, we're only aware of a few, rarely used features of libuv that are not available in sockets. +1 to what @halter73 said - we expect transport configuration to be an uncommon option and 99% of users will be fine with the default.

Kestrel is a cross-platform [web server for ASP.NET Core](xref:fundamentals/servers/index) based on [libuv](https://github.com/libuv/libuv), a cross-platform asynchronous I/O library. Kestrel is the web server that's included by default in ASP.NET Core project templates.
::: moniker-end
::: moniker range=">= aspnetcore-2.1"
Kestrel is a cross-platform [web server for ASP.NET Core](xref:fundamentals/servers/index) based on managed sockets (`Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets`), a cross-platform asynchronous I/O library. In versions of ASP.NET Core prior to 2.1, Kestrel is based on [libuv](https://github.com/libuv/libuv).

This comment has been minimized.

@halter73

halter73 May 3, 2018

Member

I think we should leave discussion of libuv and managed sockets transports out of the intro. Mentioning libuv before wasn't so bad because it was short, but now it seems overly technical for the intro paragraph. This should be irrelevant to the vast majority of ASP.NET developers.

This comment has been minimized.

@guardrex

guardrex May 3, 2018

Collaborator

ok ... I'll address that on the next commit.

This comment has been minimized.

@halter73

halter73 May 3, 2018

Member

I don't know how detailed we want this paragraph to be, but I think something like "Kestrel is a cross-platform web server for ASP.NET Core that's included by default in ASP.NET Core project templates" is enough.

@guardrex

This comment has been minimized.

Collaborator

guardrex commented May 3, 2018

RE: The new section

Are we all on the same page?

  • Leave this in it's new section
  • Don't sweat more content than what I have
* [Kestrel](xref:fundamentals/servers/kestrel) is a cross-platform HTTP server based on [libuv](https://github.com/libuv/libuv), a cross-platform asynchronous I/O library.
::: moniker-end
::: moniker range=">= aspnetcore-2.1"
* [Kestrel](xref:fundamentals/servers/kestrel) is a cross-platform HTTP server based on managed sockets (`Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets`), a cross-platform asynchronous I/O library. In versions of ASP.NET Core prior to 2.1, Kestrel is based on [libuv](https://github.com/libuv/libuv).

This comment has been minimized.

@halter73

halter73 May 3, 2018

Member

I also don't think discussion of the transport belongs here. If we must include it, I wouldn't call Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets a cross-platform asynchronous I/O library. That's what libuv is, but the sockets transport is more limited. It's just a socket library that implements Kestrel's transport abstraction.

This comment has been minimized.

@guardrex

guardrex May 3, 2018

Collaborator

Sure, I can address that on the next commit.

This comment has been minimized.

@halter73

halter73 May 9, 2018

Member

I know we go into this in detail in the kestrel-specific doc, but I think it's still worth mentioning libuv is still an option for 2.1 here.

This comment has been minimized.

@halter73

halter73 May 9, 2018

Member

That, or just remove the transport discussion from the main server doc.

This comment has been minimized.

@guardrex

guardrex May 9, 2018

Collaborator

Yes, I think we should remove it. If I try to add that sentiment, it becomes perhaps a bit too complicated ...

Kestrel is a cross-platform HTTP server based on managed sockets (Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets), a cross-platform asynchronous I/O library. In versions of ASP.NET Core prior to 2.1, Kestrel is based on libuv, which remains a viable alternative for 2.1 or later apps.

We can just go with this here ...

Kestrel is a cross-platform HTTP server based on managed sockets (Microsoft.AspNetCore.Server.Kestrel.Transport.Sockets), a cross-platform asynchronous I/O library.

@halter73

This comment has been minimized.

Member

halter73 commented May 3, 2018

@guardrex I think the content you've added already is enough. I would just keep the old intros more-or-less with the libuv stuff removed.

@guardrex

This comment has been minimized.

Collaborator

guardrex commented May 3, 2018

@Rick-Anderson Would you take a look and see if u feel I should cut/move content? ... particularly the last paragraph.

@Rick-Anderson

This comment has been minimized.

Contributor

Rick-Anderson commented May 3, 2018

@guardrex I'd go with what @halter73 suggests.

@guardrex

This comment has been minimized.

Collaborator

guardrex commented May 3, 2018

ty @Rick-Anderson

@halter73 I think the last commit addresses your feedback ... let me know if not, and I'll square it away.

@natemcmaster I can move that bit (or another author will compose it) as soon as we get App package coverage (possibly migration-to-2.1 coverage, too) in place.

@guardrex

This comment has been minimized.

Collaborator

guardrex commented May 9, 2018

How are we on engineering approval here? Are we good?

I can move that last paragraph regarding the metapackages situation to the new "App" package content being added by #6069 as soon as that merges. However, I think we should push this forward as-is, and I'll open a new issue to move the paragraph shortly.

@halter73

I left one more comment because I don't like how the new language implies that libuv is no longer supported in 2.1, but overall it looks good.

```xml
<PackageReference Include="Microsoft.AspNetCore.Server.Kestrel.Transport.Libuv"
Version="2.1.0" />
```

This comment has been minimized.

@halter73

halter73 May 9, 2018

Member

One more thing. Can we have a one-line snippet like this, but for the actual call to UseLibuv()?

@guardrex

This comment has been minimized.

Collaborator

guardrex commented May 9, 2018

@halter73

one-line snippet

How about a bit more context? ... I've learned the hard way from readers that they really don't like one liners all that much ... they get lost and don't know where to place the line. See the last commit.

[EDIT] I added two options. Show one? If so, which one? Or show both? ... or do u still want to go with a one-liner?

guardrex added some commits May 9, 2018

guardrex added some commits May 10, 2018

@@ -20,14 +20,13 @@ An ASP.NET Core app runs with an in-process HTTP server implementation. The serv
ASP.NET Core ships two server implementations:
* [Kestrel](xref:fundamentals/servers/kestrel) is a cross-platform HTTP server based on [libuv](https://github.com/libuv/libuv), a cross-platform asynchronous I/O library.

This comment has been minimized.

@halter73

halter73 May 10, 2018

Member

Is this a 1.x section? If not, I'd just drop everything after and including "based on". Maybe you could mention it's the default server, but that's covered almost immediately after this already.

WebHost.CreateDefaultBuilder(args)
.UseLibuv()
.UseStartup<Startup>();
}

This comment has been minimized.

@halter73

halter73 May 10, 2018

Member

This looks great. I think the first version is enough since that's what most people see in their templates these days.

@guardrex

This comment has been minimized.

Collaborator

guardrex commented May 10, 2018

ty @halter73 ... the first one was just exhaustion taking over. Updates made.

@guardrex guardrex requested a review from scottaddie May 10, 2018

```csharp
var host = new WebHostBuilder()
.CaptureStartupErrors(true)
...
```
---
* * *

This comment has been minimized.

@scottaddie

scottaddie May 10, 2018

Member

The tab specification doc calls for 3 consecutive dashes. Let's stick with that.

This comment has been minimized.

@guardrex

guardrex May 10, 2018

Collaborator

I just saw these start to change somewhere ... I thought they were going for a new format ... along with the four hash tabs.

Where did I see this coming in? I'll research and see where I got this.

This comment has been minimized.

@guardrex

guardrex May 10, 2018

Collaborator

It was this one ... #5878

This comment has been minimized.

@guardrex

guardrex May 10, 2018

Collaborator

These were changed in a lot of spots (but not everywhere).

```csharp
var host = new WebHostBuilder()
.UseContentRoot("c:\\mywebsite")
...
```
---
* * *

This comment has been minimized.

@scottaddie

scottaddie May 10, 2018

Member

Convert to 3 dashes.

}
```
No action is required when the [Microsoft.AspNetCore.All](xref:fundamentals/metapackage) metapackage is used and `UseLibuv` is called because the Libuv transport package is transitively referenced by the `Microsoft.AspNetCore.All` metapackage. Note that the ASP.NET Core 2.1 or later templates reference the [Microsoft.AspNetCore.App](https://www.nuget.org/packages/Microsoft.AspNetCore.App/) metapackage by default. The use of the `Microsoft.AspNetCore.All` metapackage isn't recommended for ASP.NET Core 2.1 or later apps.

This comment has been minimized.

@scottaddie

scottaddie May 10, 2018

Member

Since this section only displays for ASP.NET Core 2.1+, we shouldn't reference the older ".All" metapackage. That will really slim down this paragraph.

This comment has been minimized.

@guardrex

guardrex May 10, 2018

Collaborator

I think it would be best in a migration topic, but I don't see TO-2.1 content in the migration area.

This para has taken a lot of heat. 😄 lol I'd prefer to cut it at 3.0; but since everyone else dislikes it, I'll cut it.

@guardrex guardrex merged commit 0f0cd06 into master May 10, 2018

2 checks passed

OpenPublishing.Build Validation status: passed
Details
license/cla All CLA requirements met.
Details

@guardrex guardrex deleted the guardrex/kestrel-libuv-updates branch May 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment