DataAction / AdoNetCore.AseClient

AdoNetCore.AseClient - a .NET Core DB Provider for SAP ASE
Apache License 2.0
106 stars 44 forks source link

Fixed so PoolSize cant be negative. "Fixes #158". Made sure pooling not … #159

Closed ts46235 closed 4 years ago

ts46235 commented 4 years ago

…kicked off if _parameters.Pooling==false

senseibaka commented 4 years ago

Interesting. Since TryFillPoolToMinSize increments PoolSize, it would be good to know where the race condition is that results in negative PoolSize...

This PR should prevent the value going negative, which is nice, but it would also be good to track down and solve the root cause... Do you have any thoughts?

ts46235 commented 4 years ago

image

In our app we are running a health check every 30 seconds. When it tries to GetNewConnection it fails and catches the thrown exception and calls RemoveConnection(), which decrements PoolSize. This is outside of the loop that runs in ConnectionPool (which increments/decrements PoolSize) and in this flow there are no increments, just decrements so I assume that is the underlying issue.

try
            {
                Logger.Instance?.WriteLine($"{nameof(CreateNewPooledConnection)} start");
                return await _connectionFactory.GetNewConnection(cancellationToken, eventNotifier);
            }
            catch
            {
                Logger.Instance?.WriteLine($"{nameof(CreateNewPooledConnection)} failed");
                RemoveConnection();
                throw;
            }
senseibaka commented 4 years ago

Ah, I think I see!

It looks like there's a doubling-up of RemoveConnection calls, since TryFillPoolToMinSize (and TryReplaceConnection, too), and CreateNewPooledConnection are both calling it...

ts46235 commented 4 years ago

Correct.

But I could not recreate the issue when the first DB request was made with DB network access and then losing network access though...only when the app started without being able to access the DB.

You can update my PR or make a new one if you think you need to address that, but either way we are eager to get a new release integrated so our DBA stops being mad at us :)

senseibaka commented 4 years ago

@ts46235 Sorry, I'm unable to do a release at the moment, being the time of the year that it is. I'm hoping to do a release while I'm on holiday break.

In the meantime however, you do still have the option of using a build from your fixed branch