search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.11k stars 4.7k forks source link

Future of System.Data.SqlClient for 5.0 #30772

Closed Anipik closed 4 years ago

Anipik commented 5 years ago

This issue is mainly to discuss the future of System.Data.SqlClient package. Currently it is shipped as an out of box package and public surface area of this freezed.

Do we want to keep shipping this package in 5.0.0 or should we just want to remove this from 5.0 onward ?

cc @cheenamalhotra, @Gary-Zh , @david-engel, @safern @ericstj

danmoseley commented 5 years ago

There was a thread a while back - one thing to consider is that if we will support this longer than we support 3.x (I assume this is the case because it will ship indefinitely) then we may need to service it after the 3.x branch and servicing infrastructure no longer exists.

Wraith2 commented 5 years ago

There is active work to extend the ADO api, for example https://github.com/dotnet/corefx/issues/27682 , and when that moves forward the corefx version of SqlClient should support that api if the only thing it does is ship with obsolete messages/NSE's pointing at M.D.SqlClient

I'd say that it's also worth keeping for bug fixes alone. I've a couple of PR's open on it for bugs and I know there are more in there that will bite people. It's the first version of SqlClient that supports non-windows and having shipped so closely aligned to the framework it'll be in a lot of places that will presumably benefit from those bug fixes.

ViktorHofer commented 5 years ago

There is active work to extend the ADO api, for example dotnet/runtime#25297 , and when that moves forward the corefx version of SqlClient should support that api if the only thing it does is ship with obsolete messages/NSE's pointing at M.D.SqlClient

For how long would we add APIs to the legacy package pointing to the new one? This sounds problematic to me.

jkotas commented 5 years ago

From https://devblogs.microsoft.com/dotnet/introducing-the-new-microsoftdatasqlclient/ :

Microsoft.Data.SqlClient will be the only place we will be implementing new features going forward. We would encourage you to evaluate your needs and options and choose the right time for you to migrate your application or library from System.Data.SqlClient to Microsoft.Data.SqlClient.

@Wraith2 I would recommend you start submitting your bug fixes and performance improvements to Microsoft.Data.SqlClient. Even if System.Data.SqlClient stays in CoreFX, I expect we will be hesitant to take significant changes because of risk. CoreFX does not have the infrastructure to fully validate significant changes in System.Data.SqlClient as we have learned the hard way in https://github.com/dotnet/corefx/issues/40476

pawelpabich commented 5 years ago

From https://devblogs.microsoft.com/dotnet/introducing-the-new-microsoftdatasqlclient/ :

It means the development focus has changed. We have no intention of dropping support for System.Data.SqlClient any time soon. It will remain as-is and we will fix important bugs and security issues as they arise. If you have a typical application that doesn’t use any of the newest SQL features, then you will still be well served by a stable and reliable System.Data.SqlClient for many years.

Is there a chance that bug fixes will be applied to both S.D.SqlClient and M.D.SqlClient? M.D.SqlClient only shipped a week or so ago. I hope you understand that switching to the new provider is not a simple task.

jkotas commented 5 years ago

Is there a chance that bug fixes will be applied to both S.D.SqlClient and M.D.SqlClient?

The blog post has answer to this too. From https://devblogs.microsoft.com/dotnet/introducing-the-new-microsoftdatasqlclient/ :

We have no intention of dropping support for System.Data.SqlClient any time soon. It will remain as-is and we will fix important bugs and security issues as they arise.

pawelpabich commented 5 years ago

My comment wasn't precise enough. The blog post talks about fix **important** bugs whereas I am talking about all bugs.

We are running .NET Core 2.2 on Linux in AKS backed by Azure SQL and only a week ago we managed to find a version of S.D.SqlClient (4.7.0-preview6.19303.8) that doesn't crash our app. We've tried more recent versions but they had an unfortunate regression.

In our experience S.D.SqlClient isn't yet rock-solid on (AKS + Linux + Azure) so I hope this explains why I find fix important bugs worrying. Can you share what you understand by important bugs here? That might help.

jkotas commented 5 years ago

We've tried more recent versions but they had an unfortunate regression.

Have you tried the most recent nightly version with a fix for dotnet/runtime#30645? Does it work on AKS + Linux + Azure?

The important bugs are the same type of bugs as what we are fixing in servicing releases: key scenario not working (ASK + Linux + Azure falls into this category), security issues, reliability issues (crash, hang, data loss).

Wraith2 commented 5 years ago

The important bugs are the same type of bugs as what we are fixing in servicing releases: key scenario not working

Like mars on non-windows being unstable (https://github.com/dotnet/corefx/pull/38271) and command cancellation just not working (https://github.com/dotnet/corefx/pull/38266) which are bug fixes have been open for 3 months, easily in time for 3.0 and yet remain unmerged.

There are politics at work with SqlClient and if support/attention is reduced further i can't see even important bugs being patched. It'll be less trouble to take the honest route and just mark it as deprecated then point users at the Microsoft.Data.SqlClient

pawelpabich commented 5 years ago

Have you tried the most recent nightly version with a fix for dotnet/runtime#30645? Does it work on AKS + Linux + Azure?

We've spent quite a bit of time finding the current working for us version so at this stage we will wait for 4.7 to RTM with .NET Core 3.0.

jkotas commented 5 years ago

command cancellation not working

This does not qualify as important bug fix for me. We would not take this fix in servicing release either. This is the kind of fix that I expect to be made in Microsoft.Data.SqlClient only.

Wraith2 commented 5 years ago

It wasn't intended for a servicing release, as i said it's been sat open for 3 months which was more than enough time to get it included in the standard build before 3.0 cutoff. It's just that no-one was interested.

If it's going to get even harder to make any change to SqlClient than it already is then stop pretending to support it in corefx and mark it as dead, Just tell people who find bugs to move to M.D.SqlClient and don't mess consumers around by exposing whatever disconnect is going on internally.

jkotas commented 5 years ago

@Wraith2 I have submitted dotnet/corefx#40946 to make this clear.

jkotas commented 4 years ago

one thing to consider is that if we will support this longer than we support 3.x (I assume this is the case because it will ship indefinitely) then we may need to service it after the 3.x branch and servicing infrastructure no longer exists.

There are several other packages that are shipping from 3.x branch, that are not building for .NET 5 and that may be supported "indefinitely". So this is not introducing a new problem.

ericstj commented 4 years ago

If that became a burden we could even change the build workflow to build and test a smaller part of the product.

jkotas commented 4 years ago

Fixed by https://github.com/dotnet/runtime/pull/2275