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

Update in-memory database to use optimistic offline lock for IsRowVersion concurrency #10625

Closed
aidapsibr opened this issue Jan 1, 2018 · 14 comments
Labels
closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. customer-reported

Comments

@aidapsibr
Copy link

aidapsibr commented Jan 1, 2018

The original issue here was about support for in the in-memory database for both:

  • Concurrency tokens and the [ConcurrencyCheck] attribute
  • Automatic concurrency checking similar to rowversion/timestamp on SQL Server

As of the 2.1 release, the first of these is now implemented. This issue remains open to track implementation of the second.


Original issue:

InMemory provider does not appear to respect ConcurrencyCheck or RowVersion.

Steps to reproduce

    public class ConcurrencyCheckTests
    {
        [Fact]
        public async Task ProveConcurrencyCheckIsNotUsed()
        {
            var id = Guid.NewGuid();

            using (var dbContext = new SampleDbContext())
            {
                dbContext.Data.Add(new Datum
                {
                    Id = id,
                    Counter = 0,
                    CommitTag = Guid.NewGuid()
                });

                await dbContext.SaveChangesAsync();
            }

            await Assert.ThrowsAsync<DbUpdateConcurrencyException>(async () =>
            {
                var t1 = Task.Run(async () => await ModifyDataAsync());
                var t2 = Task.Run(async () => await ModifyDataAsync());
                var t3 = Task.Run(async () => await ModifyDataAsync());
                var t4 = Task.Run(async () => await ModifyDataAsync());

                await Task.WhenAll(t1, t2, t3, t4);
            });

            using (var dbContext = new SampleDbContext())
            {
                var entity = await dbContext.Data.SingleAsync(datum => datum.Id == id);

                Assert.Equal(4, entity.Counter);
            }

            async Task ModifyDataAsync()
            {
                using (var dbContext = new SampleDbContext())
                {
                    var entity = await dbContext.Data.SingleAsync(datum => datum.Id == id);

                    entity.Counter++;
                    entity.CommitTag = Guid.NewGuid();

                    await dbContext.SaveChangesAsync();
                }
            }
        }

    }

    public class SampleDbContext
        : DbContext
    {
        public DbSet<Datum> Data { get; set; }

        protected override void OnModelCreating(ModelBuilder modelBuilder)
        {
            base.OnModelCreating(modelBuilder);

            modelBuilder.Entity<Datum>()
                .HasKey(datum => datum.Id);

            modelBuilder.Entity<Datum>()
                .Property(datum => datum.CommitTag)
                .IsConcurrencyToken();
        }

        protected override void OnConfiguring(DbContextOptionsBuilder optionsBuilder)
        {
            base.OnConfiguring(optionsBuilder);

            optionsBuilder.UseInMemoryDatabase("Sample");
        }
    }

    public class Datum
    {
        public Guid Id { get; set; }

        public int Counter { get; set; }

        public Guid CommitTag { get; set; }
    }

This fails with the following output:

Expected: typeof(Microsoft.EntityFrameworkCore.DbUpdateConcurrencyException)
Actual:   (No exception was thrown)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter`1.GetResult()
   at REstate.Engine.Repositories.SqlServer.Tests.ConcurrencyCheckTests.<ProveConcurrencyCheckIsNotUsed>d__0.MoveNext() in C:\Users\Ovan\source\REstate\test\REstate.Engine.Repositories.SqlServer.Tests\Class1.cs:line 27
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)

Furthermore, if I bypass the Assert.ThrowsAsync the Counter is 1, so the update set the counter to 1 four times.

Further technical details

EF Core version: 2.0.1
Database Provider: (e.g. Microsoft.EntityFrameworkCore.InMemory) 2.0.1
Operating system: Windows 10 x64 build 17075 -pre
IDE: (e.g. Visual Studio 2017 15.5.2)

@ajcvickers
Copy link
Contributor

Duplicate of #7210

@ajcvickers ajcvickers marked this as a duplicate of #7210 Jan 2, 2018
@aidapsibr
Copy link
Author

Not sure about that, I mentioned IsRowVersion since they have similar behaviors, but this is about Concurrency token primarily.

It looks like this should have been fixed in #10157. Not sure if that has shipped?

@ajcvickers
Copy link
Contributor

@psibernetic That issue is in the 2.1 milestone and so the fix has not yet been shipped. You could try with our nightly builds--feeds listed on the repo homepage: https://github.com/aspnet/EntityFrameworkCore

@ajcvickers
Copy link
Contributor

Leaving this open so we can revisit in triage whether to have an issue for IsRowVersion on in-memory.

@ajcvickers ajcvickers reopened this Jan 2, 2018
@aidapsibr
Copy link
Author

For anyone who stumbles across this issue, I confirmed DbUpdateException is thrown when using IsConcurrencyCheck() AND IsRowVersion() in 2.1.0-preview1. IsRowVersion() just doesn't auto-set values like most expect.

@ajcvickers
Copy link
Contributor

We discussed this in triage and decided that once #2195 is implemented, then in-memory database should be made to work with this. Keeping this issue open to track specifically making the in-memory database work once #2195 is done.

@ajcvickers ajcvickers changed the title InMemory provider does not appear to respect ConcurrencyCheck or RowVersion. Update in-memory database to use optimistic offline lock for IsRowVersion concurrency Jan 3, 2018
@ajcvickers ajcvickers added this to the Backlog milestone Jan 3, 2018
@SimonCropp
Copy link
Contributor

this would seem to be incorrectly titled. it is actually about IsConcurrencyToken #10625 (comment)

and the repro code does not use rowversion at all

@ajcvickers
Copy link
Contributor

@SimonCropp Correct. The code doesn't match the issue that this is tracking. But the title is correct at least! :-)

@SimonCropp
Copy link
Contributor

ok i am confused. can either the title or the repro be fixed so that they match, since from my perspective it seems that IsConcurrencyToken was conflated with row version which resulted in an incorrect title. ie "fixing" this issue based on the title will have no impact on the repro as described.

@aidapsibr
Copy link
Author

The original issue was for both, neither worked at the time. My code showed IsConcurrencyCheck because I assumed that it was used by IsRowVersion. Not sure if that is true, but there was a larger issue with row version not being supported in the in-memory db. This issue now tracks the enablement of #2195 for the in-memory implementation I think.

The repro code is no longer applicable. Should probably spin up a new issue owned by the team and reference this one.

@SimonCropp
Copy link
Contributor

SimonCropp commented Jun 25, 2018

@psibernetic thanks for the clarification.

the reason i am asking is that i am using in-memory heavily for testing, and i find some of the pieces it doesnt currently implement to be desirable. So i created a "in memory feature polyfill" (for lack of a better term) https://github.com/SimonCropp/EfCore.InMemoryHelpers

for the issues it fixes/implements, i want to track those issues, and remove the specific code related to them as they are fixed.

@aidapsibr
Copy link
Author

Very nice, thanks for sharing!

@Rick-Anderson
Copy link

The sample for Razor Pages with EF Core in ASP.NET Core - Concurrency, code at https://github.com/dotnet/AspNetCore.Docs/tree/main/aspnetcore/data/ef-rp/intro/samples/cu50 reproduces the problem with the in-memory provider. See Optional: Build the sample download and change StartupSQLite to:

        public IConfiguration Configuration { get; }

        // This method gets called by the runtime. Use this method to add services to the container.
        public void ConfigureServices(IServiceCollection services)
        {
            services.AddRazorPages();

#if !IN_MEM
            services.AddDbContext<SchoolContext>(options =>
                    options.UseSqlite(Configuration.GetConnectionString("SchoolContext")));
#else

            services.AddDbContext<SchoolContext>(options =>
        options.UseInMemoryDatabase("SchoolContext"));
#endif

            services.AddDatabaseDeveloperPageExceptionFilter();
        }

@ajcvickers ajcvickers added closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. customer-reported and removed area-save-changes labels Oct 26, 2022
@ajcvickers
Copy link
Contributor

We recommend against using the in-memory provider for testing--see Testing EF Core Applications. While we have no plans to remove the in-memory provider, we will not be adding any new features to this provider because we believe valuable development time is better spent in other areas. When feasible, we plan to still fix regressions in existing behavior.

@ajcvickers ajcvickers closed this as not planned Won't fix, can't repro, duplicate, stale Oct 26, 2022
@ajcvickers ajcvickers removed this from the Backlog milestone Oct 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed-out-of-scope This is not something that will be fixed/implemented and the issue is closed. customer-reported
Projects
None yet
Development

No branches or pull requests

5 participants