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

Déjà vu #2392

Merged
merged 2 commits into from
Mar 14, 2018
Merged

Déjà vu #2392

merged 2 commits into from
Mar 14, 2018

Conversation

halter73
Copy link
Member

@halter73 halter73 commented Mar 14, 2018

Default to the managed socket transport

#2360

@@ -17,9 +17,6 @@
<ItemGroup>
<ProjectReference Include="..\Kestrel.Core\Kestrel.Core.csproj" />
<ProjectReference Include="..\Kestrel.Https\Kestrel.Https.csproj" />

<!-- Even though the Libuv transport is no longer used by default, it remains for back-compat -->
<ProjectReference Include="..\Kestrel.Transport.Libuv\Kestrel.Transport.Libuv.csproj" />
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@natemcmaster I left this comment the first time I switched the default transport to managed sockets. Doesn't removing this dependency break back-compat?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the break only we're willing to take for 2.1.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be specific, I think this change will break any existing app that references just the main Kestrel package and calls UseLibuv.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is an intentional breaking change that we decided is worth taking. It should only affect a small group of people since we are keeping Transport.Libuv in the .All metapackage. It only breaks for people who depend directly on the .Kestrel package, and they can fix this break by adding a package reference to K.T.Libuv when they upgrade to 2.1. IMO we should post this to aspnet/announcements as a heads up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that's known. This will be documented as a breaking change in 2.1.

/cc @DamianEdwards @Eilon

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you breaking semantic versioning by introducing breaking change in a minor version increment?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because of the reasons listed here #2360. A few things are changing in 2.1 as well. We’re introducing a shared framework for ASP.NET Core and we’d prefer it if libuv wasn’t part of that. Also we feel like it will break few enough people that it shouldn’t matter that much. The workaround for people is also trivial and if you’re upgrading from ASP.NET Core 2.0 to 2.1 and you’re using the .All package everything still works.

Where things will break is if you were using .net Framework with libuv and were configuring the API explicitly (to say tweak the number of threads)

@davidfowl
Copy link
Member

@halter73 can you also stage various reaction PRs to other repositories using a feature branch (if there are any). I'll send a PR to benchmarks that explicitly adds the libuv package.

@halter73
Copy link
Member Author

halter73 commented Mar 14, 2018

Absolutely! I'll get right to staging all the other reaction PRs 😉

@davidfowl
Copy link
Member

:trollface:

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

Successfully merging this pull request may close these issues.

6 participants