Improve how ConnectionAcquisitionTimeout is checked #233
Conversation
…ol and converted recursive calls to loops in order to avoid any possible stack overflows.
| return connection; | ||
| }); | ||
| } | ||
| catch (OperationCanceledException ex) |
There was a problem hiding this comment.
I am thinking that this try-catch should be inside return TryExecute. TryExecute is only used to log errors before throwing the error to user. I would think this OperationCanceledException is less interesting to an end user than ClientException
| break; | ||
| } | ||
|
|
||
| if (_availableConnections.TryTake(out connection, 0, cancellationToken)) |
There was a problem hiding this comment.
Why removed ConnAcquisitionQueuingTimeout? We initially want this blocking queue is to make thread wait a bit on this TryTake method to introduce some queuing/waiting inside async. I am a bit worried if we return directly without any sleep, will the thread span here a lot?
There was a problem hiding this comment.
I've put a small time lapse back into the loop (not a magnitude of seconds but milliseconds) but remove those timeouts and cancellationtoken checking logic from the first TryTake - making the fast path do not bother with those.
| { | ||
| break; | ||
| ; |
| { | ||
| if (!IsConnectionPoolFull()) | ||
| { | ||
| connection = CreateNewPooledConnection(); |
There was a problem hiding this comment.
Unrelated to this PR: given a pool with maxConnectionPoolSize - 1 acquired and 0 idle connections, can two threads that call Acquire() concurrently make pool expand to maxConnectionPoolSize + 1 active connections? They will both try to create concurrently and attempts are probably not synchronized.
There was a problem hiding this comment.
Except for correctness, I am not fan to strictly not exceed maxConnPoolSize. As it not only slows the code down also bring more complete logic.
There was a problem hiding this comment.
Suggesting to have the fix in another PR. Right now the scope of this PR is good enough and the problem is not critical to block 1.5.
|
I think I've addressed the synchronization issue you've pointed out. |
| { | ||
| if (_poolSize > _maxPoolSize) | ||
| { | ||
| throw new InvalidOperationException(); |
There was a problem hiding this comment.
Should we also decrement _poolSize here?
| @@ -151,6 +149,17 @@ public IPooledConnection CreateNewPooledConnection() | |||
| private PooledConnection NewPooledConnection() | |||
| { | |||
| Interlocked.Increment(ref _poolSize); | |||
| if (_maxPoolSize != Config.Infinite && _poolSize > _maxPoolSize) | |||
There was a problem hiding this comment.
If we have two threads and _poolSize == _maxPoolSize - 1 they both will increment _poolSize and throw, right? I think one of them should succeed. We could probably use result of Interlocked.Increment to figure out which one should fail.
No description provided.