dotnet / EntityFramework.Docs

Documentation for Entity Framework Core and Entity Framework 6
https://docs.microsoft.com/ef/
Creative Commons Attribution 4.0 International
1.62k stars 1.96k forks source link

Be less insistent that people should not use AddAsync in most cases #4320

Open user1568891 opened 1 year ago

user1568891 commented 1 year ago

C# developers are generally trained to always use "async" versions of methods. However, the Entity Framework AddAsync method according to the docs shouldn't always be used except in certain situations. This is because AddAsync is apparently not just an async version of Add. This unintuitive design leads to inefficient code and endless arguments.

Please rename the AddAsync method to be called something like AddAndFetchValueGeneratorsAsync. This will make the purpose and when you should use it much more obvious.

roji commented 1 year ago

However, the Entity Framework AddAsync method according to the docs shouldn't always be used except in certain situations.

That's not true; there's no particular reason to avoid AddAsync. The performance hit of using AddAsync instead of Add - when no I/O needs to be performed - is negligible in the vast majority of cases. It isn't really needed except in certain situations, but that means there's any problem ith using it outside of those situations.

This is because AddAsync is apparently not just an async version of Add.

It is exactly that. In the event that I/O needs to happen while adding (i.e. because of value generation), Add performs that I/O synchronously whereas AddAsync performs it asynchronously.

In any case, renaming AddAsync would needlessly break a huge amount of existing code for no good reason.

user1568891 commented 1 year ago

True or not, its what the documentation currently says here and when you hover over the method in Visual Studio: https://learn.microsoft.com/en-us/ef/core/change-tracking/miscellaneous#add-versus-addasync

So either the documentation needs to be updated, or the method should be renamed to reflect its use.

As "for no good reason", just because you don't agree doesn't mean you have to be dismissive of my opinion. In fact you proved my point about it causing endless arguments.

There are also several blogs around the internet promoting the idea to not use AddAsync, so I'm not the only one with that belief. That alone suggests that at the very least the documentation should be updated to say that either is fine.

It could be marked as obsolete for a while to avoid "breaking a huge amount of code".

roji commented 1 year ago

True or not, its what the documentation currently says here and when you hover over the method in Visual Studio

I've read our docs which you linked to, and I simply don't see what you're saying. The closest seems to be:

This means that the vast majority of applications should use Add, and not AddAsync.

I think that's correct advice, though it's also completely fine to use AddAsync instead. The docs don't say that there's any problem in using AddAsync, and don't even mention that there's a perf overhead associated with calling AddAsync instead of Add (an overhead which again, would be negligible for all but very high-perf scenarios).

As "for no good reason", just because you don't agree doesn't mean you have to be dismissive of my opinion.

Where do you feel that my comment was dismissive? I simply don't agree with what you're saying, and also pointed out that a statement you made is false ("AddAsync is apparently not just an async version of Add").

In fact you proved my point about it causing endless arguments.

I'm not at all aware of "endless arguments" around this - here in this repo we certainly don't see that. In any case, the fact I don't agree with a user on some point doesn't imply that there are endless arguments and that something needs to be changed.

Stepping back, this "problem" isn't specific to EF in any way, and exists anywhere where there's an abstraction over something that may or may not perform I/O. For example, EF also exposes both SaveChanges and SaveChangesAsync; in the vast majority of databases, networking has to happen to communicate with the database, and therefore SaveChangesAsync should be used in a scalable application etc.

However, the SQLite driver has no asynchronous support; executing SQL involves calling a native function in synchronous fashion. In other words, if you've chosen SQLite as your EF (or even ADO.NET) database provider, there's no point in calling SaveChangesAsync - under the hood everything is performed synchronously in any case. So in a very perf-sensitive application, it may make sense to use SaveChanges to avoid the async method overhead, since you know that behind the abstraction (EF and its SaveChanges) is a provider that will never work asynchronously. That, in itself, isn't a reason to rename SaveChangesAsync to something else, like AddAsync shouldn't need to be renamed just because in certain (or even most) cases it won't do I/O.

It could be marked as obsolete for a while to avoid "breaking a huge amount of code".

We don't do that unless there's a serious problem with an API, which is causing issues for our users; that's not the case here at all. We can indeed tweak the docs a bit, but I really haven't seen evidence that it's a problem for people.

chase-miller commented 1 month ago

This means that the vast majority of applications should use Add, and not AddAsync.

Thoughts on updating the above to the following?

In the vast majority of cases - where no I/O is performed - the overhead of using AddAsync is negligible compared to Add due to {insert reason}.

I find providing this information more valuable than the current suggestion that is devoid of characterizing AddAsync’s overhead. One could argue that the majority of consumers should in fact use AddAsync to avoid potential future issues unless there’s a good reason not to ({insert use cases here}).