Sql Zombie transactions about identity.dapper HOT 34 CLOSED

Maybe we should use a UnitOfWork and work with TransactionScope?
I'll take a look when i have time.

from identity.dapper.

What is the message of exception?

from identity.dapper.

Message is sometimes : "SqlConnection does not support parallel transactions." and sometimes somethinga bout a Zombie connection. I think we'll have to make the connections as a 1-per-query. As far as being able to commit and rollback the transactions, we would either have to make the transaction a 1-per-call (which I think would be a giant pain) or 1-per-query. But then again, transactions on 1-per-query are all much pointless...

from identity.dapper.

I can start working on setting the connections as 1-per-query if you want. Days have been pretty slow lately lol.

from identity.dapper.

I think we should investigate first if this behavior of SqlConnection is just for .NET Core or on full framework too.

I'll check the code before we do something, i've to remember some things and try to elucidate the architecture for this bug.

from identity.dapper.

Agreed. I do know of an enterprise level solution running dapper on .Net Core where all of the repos are written in this format:

public IEnumerable<User> GetAllUsersWithoutRoles()
{
    try
    {
        using (var conn = Getconnection())
        {
            const string query = @"
                Select t1.* 
                From [Users] t1
                    left Join [UserRoles] t2 on t1.Id = t2.UserId 
                Where 
                    t2.UserId is null";
             var users = conn.Query<User>(query);
             return users;
        }
    }
   catch (Exception ex)
   {
       _log.LogError(new EventId(123), ex.Message, ex);
       return null;
    }
}
protected SqlConnection GetConnection()
{
    var conn = new SqlConnection(_myDB);
    conn.Open();
    return conn;
}   

That doesn't exactly answer the question but if needed it is a good template. I noticed these aren't async db calls, don't know why though.

from identity.dapper.

Weird that i've a enterprise system running the same way with transactions with a lot more requests and this error does not throw.
The difference it's running on full Framework.

This need investigation.
Maybe we arent disposing the transactions properly.

from identity.dapper.

Here is a little more background as it may help with the investigation...

I first started noticing problems with the requests (same endpoints) taking too long. When I went to look at the db I saw that my UserRoles table was totally locked. I had to manually kill operations in SSMS in order to free up the table The tables were on a sql server express db so I created my tables on a full sql server db and started again. I no longer saw the table lock issue but instead ran into the Zombie transaction issue.

from identity.dapper.

The transactions are not being disposed i guess.
I dont know if i'll have time to take a look this week, but the design of transaction probably will have to be changed.

from identity.dapper.

@nurdyguy what the simplest way to test this behavior from my computer? I think there's something with AddSingleton from ConfigureDapperXXXConnectionProvider. If you replace with AddScoped the problems go away?

from identity.dapper.

I've digged a little more on this...
So far, we'll have to reimplement everything envolving transactions.

The easiest way is to not use transactions at all, and all will runs fine.

If we want to keep transactions, we'll have to implement a way to use a single connection among the two stores (DapperUserStore and DapperRoleStore) and a single transaction until it's commited.

from identity.dapper.

Single connection is generally a bad thing. Personally I don't really like transactions (as they inherently lock db tables) but I can see that as a package some people might want to use them. Thus, from a package point of view, I'm not sure how you want to proceed. The only other option I could think of was creating a lock around the transaction so it can be properly disposed at the end. That sounds quite messy though.

For your previous post asking about how to test, the code I made (which found the issue) wasn't complicated but also wasn't simple. You could boil it down though I guess. Like I said originally, I created 5 users and 3 possible roles. Then I had a relatively simple function which just adds/removes roles from each of the users. The code for that is in the original post but you could boil it down to

• user1 remove "Owner" role and add "Admin" role
• user2 remove "User" role and add "Admin" role
• user3 remove "Admin" role and add "Owner" role
  ....

Hard code a few of those in and run it a few times. This is a race condition so it may not occur every time.

from identity.dapper.

I've reproduced and have been working on a separate branch.
So far, the current workaround is to take off the transactions.

I'll not work on a UnitOfWork with ADO.NET BeginTransaction because it would be so hard to do. I'll wait for TransactionScope works on .NET Core, because it'll be SOOOO much easier to implement.

You can check on https://github.com/grandchamp/Identity.Dapper/tree/uow

from identity.dapper.

I gave it a quite test run, no issues so far :-)

One thing though, you left non-generic info in the connection string, appsettings.json. You might want to strip that out before you merge this into master.

from identity.dapper.

Can you think in something better for transactions until TransactionScope?

from identity.dapper.

Well, here is how I always think of transactions. If I have 3 things to do, the transaction guarantees that ALL 3 happen, or NONE happen. It keeps me from getting into an invalid state where the first thing happened but then an error occurred and the second two failed. To that end, it is somewhat unnecessary in the majority of these queries. Most of the queries have only 1 operation, inserting a single record for example. If the insert fails then there is nothing to rollback because the only 1 operation didn't happen.

I could see more value in transactions at a store level rather than at a repo level. The store method may say "create this user, then do xxx with him, then do yyy with him" and thus a transaction (and maybe a retry of some sort) could/would be beneficial. But that would also require an instanced transaction being passed into the repo and disposed of back in the store.

As far as what you have now, in the uow branch, assuming it doesn't have any race conditions and/or async issues, I think it is good as is.

from identity.dapper.

But if you keep the way It is, when the third operation happens (even a select), the connection times out because the transaction.
If you do two insert operations inside the uow, you can repro this. Go to Manager controller to the method AddRole and you will see It happen.

from identity.dapper.

I didn't run into any problems with my original code (in original post) when I added and removed roles for each of 4 users. I'll try to repro as you described though.

from identity.dapper.

There are no controllers in the repo. Can you post the code for the method you mentioned?

from identity.dapper.

https://github.com/grandchamp/Identity.Dapper/blob/uow/samples/Identity.Dapper.Samples.Web/Controllers/ManageController.cs#L341

from identity.dapper.

Ahh, sorry, I forgot about your sample projects. I'll work on retro-fitting those into my solution so I can test it. Right now I'm betting a DI issue in my project so I gotta solve that first.

from identity.dapper.

I wired this up using postman and hit it about 10 times, no problems. I even added about 6 roles to the user so the Remove function had to process several at once, no problems.

In general I don't think it should be the controller's job to worry about .SaveChanges(), that should be a service layer concern. But for the purpose of testing it is fine.

from identity.dapper.

Did you set UseTransactionalBehavior = true on https://github.com/grandchamp/Identity.Dapper/blob/uow/samples/Identity.Dapper.Samples.Web/Startup.cs#L42 ?

from identity.dapper.

Of course not lol... testing again...

from identity.dapper.

Yeah, I'm having some serious issues now:

 at System.Data.SqlClient.SqlConnection.PermissionDemand()
 at System.Data.ProviderBase.DbConnectionInternal.TryOpenConnectionInternal(DbConnection outerConnection, DbConnectionFactory connectionFactory, TaskCompletionSource`1 retry, DbConnectionOptions userOptions)
  at System.Data.SqlClient.SqlConnection.TryOpen(TaskCompletionSource`1 retry)
  at System.Data.SqlClient.SqlConnection.OpenAsync(CancellationToken cancellationToken)
--- End of stack trace from previous location where exception was thrown ---
  at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
  at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
  at System.Runtime.CompilerServices.TaskAwaiter.GetResult()
  at SandboxCore.Identity.Dapper.Stores.DapperUserStore`7.<CreateTransactionIfNotExists>d__7.MoveNext() in C:\DevWork\SandboxCore\src\Identity.Dapper\Stores\DapperUserStore.cs:line 71
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.GetResult()
   at SandboxCore.Identity.Dapper.Stores.DapperUserStore`7.<RemoveFromRoleAsync>d__46.MoveNext() in C:\DevWork\SandboxCore\src\Identity.Dapper\Stores\DapperUserStore.cs:line 621
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
  at Microsoft.AspNetCore.Identity.UserManager`1.<RemoveFromRoleAsync>d__101.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
  at System.Runtime.CompilerServices.TaskAwaiter.ThrowForNonSuccess(Task task)
  at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
  at System.Runtime.CompilerServices.TaskAwaiter`1.GetResult()
  at SandboxCore.Controllers.ManageController.<AddRole>d__25.MoveNext() in C:\DevWork\SandboxCore\src\SandboxCore\Controllers\ManageController.cs:line 366

from identity.dapper.

Yep.
If you dig a bit more you'll find that after two insert operations ran on a transaction, the third operation will fail and hang.

from identity.dapper.

I stumbled on another issue too. I'm using postman and forgot to put the [FromBody] in the method signature so the object came in null which caused a null reference exception. Because that exception was thrown something didn't get disposed of somewhere which then causes issues on subsequent requests.

from identity.dapper.

One problem,

public DbConnection CreateOrGetConnection()
    {
        _semaphore.Wait();

        if (_connection == null)
        {
            _connection = _connectionProvider.Create();
            _connection.Open();

            _transaction = _connection.BeginTransaction();
        }

        _semaphore.Release();

        return _connection;
    }

There are instances where the connection is not null but it is closed. Then when you get back to DapperuserStore:

    private async Task CreateTransactionIfNotExists(CancellationToken cancellationToken)
    {
        if (!_dapperIdentityOptions.UseTransactionalBehavior)
        {
            _connection = _connectionProvider.Create();
            await _connection.OpenAsync(cancellationToken);
        }
        else
        {
            _connection = _unitOfWork.CreateOrGetConnection();

            if (_connection.State == System.Data.ConnectionState.Closed)
                await _connection.OpenAsync(cancellationToken);
        }
    }

the OpenAsync is failing. I'm not exactly sure why, but it fails nonetheless.

from identity.dapper.

So when we step through AddRole and we get to _userDataService.AddToRoleAsync(user, model.RoleName); at that point our uow is open and valid. But if you step through you eventually land in UserRepository.IsInRole. Since that is just a simple select it has its own using statement and creates its own connection. But since the transaction is still open from the previous insert, the table is locked and this new connection times out. The only way that select can run is if it is part of the same connection/transaction. Even after that though the transaction may succeed. But if I then make a second request. inspecting the uow._connection shows that the object is totally invalid, values are set to default.

from identity.dapper.

Yes.
To work with UoW, we cant dispose the connection on "non transactional" methods (basically SELECT methods). We should detect if UoW has a connection and not use using and if it hasn't, use using.

from identity.dapper.

Yeah, if you are in the middle of a transaction, everything has to use that transaction... what a pain...

There is something else wrong also but I'm having trouble hunting it down.

from identity.dapper.

That's why i'm talking that it's a pain to work on both sides now without TransactionScope.

from identity.dapper.

I think the other issue is somewhere in system garbage collection. If you make a request and it is successful, then put your debugger inside UnitOfWork.CreateTransactionIfNotExists and you'll see that the connection information is wiped. That's what was causing the error with the stack trace I posted above. Overall that shouldn't be too tough to fix. The TransactionScope issue is much bigger as you mentioned.

from identity.dapper.

Fixed in my new commits to uow branch.

Basically, i implemented the

 DbConnection conn = null;
                if (_unitOfWork?.Connection == null)
                {
                    using (conn = _connectionProvider.Create())
                    {
                        await conn.OpenAsync();

                        return await selectFunction(conn);
                    }
                }
                else
                {
                    conn = _unitOfWork.CreateOrGetConnection();
                    return await selectFunction(conn);
                }

on ALL methods, including methods with select only.

from identity.dapper.