[.NET 6 Preview 4] Command is already in progress error in Npgsql library #32254
Comments
|
Tagging subscribers to this area: @roji, @ajcvickers Issue DetailsApplication Name: ComforDev Verify Scenarios: Repro steps & source: Check at https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1319505 Repro steps: Expected Result: Actual Result: Findings : In Api, we are trying to get data from a table and also its related data with LazyLoading enabled , In Client Side , we call API : @dotnet-actwx-bot @dotnet/compat
|
|
@ajcvickers Could you please help to take a look on this to see if this belongs to system.data? |
|
@jiangzeng01 I wasn't able to reproduce the error with the repro, with the following steps:
Result: I get a proper JSON response with all the data. Note that the error you're seeing - a command is already in progress - is quite common, and especially when using lazy loading. The following snippet demonstrates it well: foreach (var blog in context.Blogs)
{
foreach (var post in blog.Posts)
{
Console.WriteLine(post.Name);
}
}In this code snippet, a command is started to fetch all the blogs. Then, before that command completes, another command is started to fetch the posts for a given blog, but two commands aren't allowed concurrently in Npgsql. This code will work on SQL Server if MARS is enabled on your connection string, which could explain why your code works on SQL Server but not on PostgreSQL. A possible workaround is to add Other unrelated comments:
|
|
@roji I'm from AppCompat team and doing third-party apps runtime combability validation with .NET 6 Preview 4 now. According to your steps, I think you have lower versions of SDK (like .NET Core 3.1 or .NET 5.0) installed in the machine, so if you do "dotnet run", app won't run on .NET 6 because it is targeting .NET Core 3.0. I think you can first do "dotnet build" under ComfortDev\ComfortDev.API and update the file ComfortDev.API.runtimeconfig under bin folder by changing "runtimeOptions" version to "6.0.0-preview.4.21222.16", then go to bin folder and "dotnet ComfortDev.API.dll". It will repro. Steps would be: This issue does not happen in main branch SDKs, but fails with Preview 4 branch SDKs. So something should be changed in Preview 4 to make it fail. |
|
Thanks for the tip @jiangzeng01, I can indeed repro this now (switching the TFM to net6.0 also works). Thanks also for providing the context, I mistakenly thought this was an actual user app. It seems like when serializing the resulting In any case, some sort of change either in System.Text.Json or in how ASP.NET Core uses it should explain this. Before digging too deeply, @davidfowl @ajcvickers does this ring any bells? Minimal error demo with lazy loadingawait using (var ctx = new BlogContext())
{
await ctx.Database.EnsureDeletedAsync();
await ctx.Database.EnsureCreatedAsync();
ctx.Blogs.Add(new()
{
Name = "Blog1",
Posts = new()
{
new() { Title = "Post1" },
new() { Title = "Post2" }
}
});
await ctx.SaveChangesAsync();
}
await using (var ctx = new BlogContext())
{
await JsonSerializer.SerializeAsync(new MemoryStream(), ctx.Blogs);
}
public class BlogContext : DbContext
{
public DbSet<Blog> Blogs { get; set; }
static ILoggerFactory ContextLoggerFactory
=> LoggerFactory.Create(b => b.AddConsole().AddFilter("", LogLevel.Information));
protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
=> optionsBuilder
.UseSqlServer(@"Server=localhost;Database=test;User=SA;Password=Abcd5678;Connect Timeout=60;ConnectRetryCount=0")
// .UseNpgsql(@"Host=localhost;Username=test;Password=test")
.UseLazyLoadingProxies()
.EnableSensitiveDataLogging()
.UseLoggerFactory(ContextLoggerFactory);
}
public class Blog
{
public int Id { get; set; }
public string Name { get; set; }
public virtual List<Post> Posts { get; set; }
}
public class Post
{
public int Id { get; set; }
public string Title { get; set; }
[JsonIgnore]
public virtual Blog Blog { get; set; }
} |
|
The repro code that @roji posted above fails for me targeting .NET 5 with EF Core 5. I can't see a way it could ever have behaved differently. |
|
@ajcvickers yeah - the minimal code snippet I posted fails for me too, regardless of .NET version. However, the original repro with ASP.NET does work (except on preview4), although it seems like it should fail in the same way... |
|
Some ideas. If the list is materialized before streaming, then this works*: await using (var ctx = new BlogContext())
{
var blogs = ctx.Blogs.ToList();
await JsonSerializer.SerializeAsync(new MemoryStream(), blogs);
}This is because now the lazy loading query is not interleaved with the regular query. So if something has changed in ASP.NET that would cause the query to be streaming now, when before it was pre-executed, then this could account for the behavior change. @mkArtakMSFT Any ideas? *When I said "works" before, I should point how that it still doesn't do what it should do, because serialization of a lazy-loading graph is a fundamentally flawed thing to do since can cause lots of additional queries to the database and a massive payload with lots of additional data loaded. |
|
@pranavkm this might be the |
pranavkm
added
the
area-mvc
|
Thanks all for you investigation and updates! |
|
Actually it's likely this change dotnet/runtime#50778 |
|
@davidfowl @pranavkm We just tried with latest main branch sdk, it also fails now. 6.0.100-preview.5.21222.19 (main branch build 2 days ago) - PASS |
|
Looking at the stacktrace, it is almost certain that the change in dotnet/runtime#50778 has contributed to the failure. The change makes types implementing both |
|
@eiriktsarpalis off the top of my hand, I'm not sure why a switch from serializing IEnumerable to IAsyncEnumerable would be important here; when lazy loading is enabled, both modes would cause additional queries to be sent when internal properties are accessed, while the main enumeration is still active (so multiple commands on the same connection at the same time, which isn't allowed). Any chance you could take a look at the code sample I posted above - which always fails - and see how it's different from what ASP.NET does when serializing? How everything was fully working pre-preview4 is probably something we need to understand (could be @ajcvickers explanation here). |
|
In 5.0, MVC buffered If we think this was a problematic pattern to start with, I think documenting the breaking behavior change and providing users correct guidance on how to do it right way would be the way to go. Btw, it's really cool we're discovering all these fun issues as part of release validation. 👍🏽 |
|
@pranavkm then yeah, makes sense and sounds pretty much like what @ajcvickers described above; the thing is less related to sync/async and more to whether ASP.NET buffers everything before the JSON serializer starts to drill into the object graph (and triggers additional queries because of lazy loading). Removing the buffering behavior sounds like the right thing, and FWIW we don't recommend using lazy loading in this way (or at all, actually...), so IMHO documenting this as a breaking change is the way to go. @ajcvickers what do you think? |
|
I think this is something ASP.NET should document as a breaking change. The lazy-loading case here is really an anti-pattern (as I mentioned above) so this failure is perhaps not very interesting. However, changing where streaming happens means that the DbContext is now used at a later point it was before. This will break if the context is disposed after where it was used before, but before where it is used now. This typically isn't a problem when the context is scoped to the request, but more and more people are using DbContextFactory, in which case the context is often disposed sooner. Likely people will now need to do their own buffering in these cases, whereas before ASP.NET was doing it for them. |
|
I agree, it is a breaking change (didn't we mark it as such?).
This is worth a discussion I think. |
@ajcvickers in this case, don't people have to dispose the context before the controller method returns, so they already have to buffer themselves in the controller? i.e. for this particular scenario I'm not sure there's a breaking change... |
mkArtakMSFT
added
feature-mvc-execution-pipeline
|
Thanks for contacting us. We're moving this issue to the |
|
@roji That's possible. |
I see that this issue has been moved to next sprint planning. In the interim, what is the plan for Preview 4? Can you please confirm whether ASPNET Core team will be creating a breaking change announcement for this or a known issue if you will decide later after investigation whether this change is by design or not? |
|
Breaking change announcement: aspnet/Announcements#463 |
ghost
locked as resolved and limited conversation to collaborators
Application Name: ComforDev
OS: Windows 10 RS5
CPU: X64
.NET Build Number: 6.0.100-preview.4.21226.5
Verify Scenarios:
1). Windows 10 RS5 X64 + 6.0.100-preview.5.21222.19 - main branch: PASS
2). Windows 10 RS5 X64 + 6.0.100-preview.4.21226.5 - Preview 4 branch: Fail
Repro steps & source: Check at https://devdiv.visualstudio.com/DevDiv/_workitems/edit/1319505
Repro steps:
1)Restore DB by psql -d ComfortDev -U postgres -f ComfortDevdb.sql
2 Start Api by dotnet ComfortDevApp\API\ComfortDev.API.dll
3) Start Client Console app to connect to API by : running ComfortDevApp\Client\ConsoleApp1.exe
Expected Result:
It should return db table record number
Actual Result:
Findings :
This issue occurs when UseLazyLoadingProxies is enabled.
We couldn't repro this issue when using Sql Server, this could only be in postgres.
In Api, we are trying to get data from a table and also its related data with LazyLoading enabled ,
In Client Side , we call API :
@dotnet-actwx-bot @dotnet/compat
The text was updated successfully, but these errors were encountered: