Skip to content
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

Please make SQL server backend async performance issue more prominent #4109

Closed
kevbry opened this issue Nov 2, 2022 · 3 comments · Fixed by #4161
Closed

Please make SQL server backend async performance issue more prominent #4109

kevbry opened this issue Nov 2, 2022 · 3 comments · Fixed by #4161
Assignees
Milestone

Comments

@kevbry
Copy link

kevbry commented Nov 2, 2022

It would be ideal if the known performance issues with using the sqlclient provider in async were noted more prominently. There is a small and easy to miss note on the general async programming docs page ( https://learn.microsoft.com/en-us/ef/core/miscellaneous/async ), but I don't think a single note in that section aligns with the potentially extreme impact of the issue.

I understand that efcore is not responsible for the underlying issue, but it is very definitely impacting users of efcore, especially given that efcore samples and tutorials make use of async patterns against the sqlclient provider and would lead users to trip over this issue. The primary intro tutorial specifically encourages users to make use of async patterns when using efcore against sql server, which is in opposition to the recommendations from the SqlClient bugs below.
https://learn.microsoft.com/en-us/aspnet/core/data/ef-rp/intro?view=aspnetcore-6.0&tabs=visual-studio#asynchronous-ef-methods-in-aspnet-core-web-apps

Reading through comments on several dozen issues raised in the efcore and sql provider repos show that many users of efcore are encountering this issue and having to spend (presumably) a lot of time tracking it down.

dotnet/SqlClient#601
dotnet/SqlClient#593


Document Details

Do not edit this section. It is required for learn.microsoft.com ➟ GitHub issue linking.

@roji
Copy link
Member

roji commented Nov 3, 2022

@kevbry what kind of "prominent message" do you have in mind? We could add another note on the SQL Server provider overview page, but I'm generally skeptical of the value of this...

For this kind of perf issue/limitations, users don't generally systematically read all the docs, and then go and write the programs, taking all of the limitations/notes into account. I understand and appreciate with the pain that this SqlClient issue causes, and with how hard it is to track down the root cause in these cases; I also share the desire to save other users this pain. But IMHO past experience has shown that adding such a note would do very little to help there; I believe users would mostly still run into the issue, and only discover the doc note after having gone through the pain of diagnosing the issue.

@kevbry
Copy link
Author

kevbry commented Nov 4, 2022

@roji That's a good point. My inclination would be to plaster it in as many places as possible, just to give people a decent chance of happening across it. As-is it isn't very discoverable being only in the github issues on sqlclient, and mostly in closed tickets in the efcore repo. With async being considered best practice for cases like this involving remote I/O also makes it a lot harder to track down, since "maybe it's because I'm using async" doesn't enter easily into mind as a possible cause.

There are a few places in the existing docs that to me seem like decent candidates

  • https://learn.microsoft.com/en-us/aspnet/core/data/ef-rp/intro?view=aspnetcore-6.0&tabs=visual-studio#asynchronous-ef-methods-in-aspnet-core-web-apps
    • The intro tutorial has an "async methods in asp.net core web apps" section that describes async efcore as a best practice. The tutorial is using sqlclient provider, and one or more models that could possibly fall victim to this issue as they'll map to nvarchar(max). While the fields in question are things like "title" that are unlikely to be long enough to cause a noticeable problem, it's probably worth a note
    • You probably have better metrics than I do, but whichever sqlclient-consuming sample projects are out there might also be good candidates for an inline comment on usages of async.
  • https://learn.microsoft.com/en-us/ef/core/performance/
    • Even though it isn't specific to sqlclient provider, this section seems like a likely spot someone would end up if they were seeing major perf problems. It's the first place I looked. My limited experience (3 independent teams) with this issue has been that everyone assumes either efcore or some environment configuration is to blame. Their assumption is that the sqlclient library wouldn't have this kind of a bug in the async path given async's "best practice" status and sqlclient being a fairly core component in a lot of codebases.

roji added a commit to roji/EntityFramework.Docs that referenced this issue Nov 25, 2022
roji added a commit to roji/EntityFramework.Docs that referenced this issue Nov 25, 2022
roji added a commit to roji/AspNetCore.Docs that referenced this issue Nov 25, 2022
@roji
Copy link
Member

roji commented Nov 25, 2022

@kevbry thanks for these suggestions!

Submitted #4161 to add notes in the SQL Server main provider page and in the async section of the querying performance docs. Also opened dotnet/AspNetCore.Docs#27747 to add the note to the ASP.NET docs as you suggested.

@roji roji self-assigned this Nov 25, 2022
@roji roji added this to the 7.0.0 milestone Nov 25, 2022
Rick-Anderson added a commit to dotnet/AspNetCore.Docs that referenced this issue Nov 28, 2022
* Add note on SqlClient async issues

See dotnet/EntityFramework.Docs#4109

* Update aspnetcore/data/ef-rp/intro.md

Co-authored-by: Rick Anderson <3605364+Rick-Anderson@users.noreply.github.com>
@roji roji closed this as completed in #4161 Dec 1, 2022
Donciavas pushed a commit to Donciavas/AspNetCore.Docs that referenced this issue Feb 7, 2024
* Add note on SqlClient async issues

See dotnet/EntityFramework.Docs#4109

* Update aspnetcore/data/ef-rp/intro.md

Co-authored-by: Rick Anderson <3605364+Rick-Anderson@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants