CSHARP-3906: Switch ConnectionPool background task to a dedicated thread. #739
Conversation
DmitryLukyanov
requested review from
JamesKovacs and
BorisDog
and removed request for
JamesKovacs
| { | ||
| _maintenanceThreadCreator((CancellationToken)cancellationToken); | ||
| } | ||
| catch (OperationCanceledException) |
There was a problem hiding this comment.
I don't think we need OperationCanceledException here, let it be maintenanceAction responsibility (it is already).
| private readonly TimeSpan _interval; | ||
|
|
||
| public MaintenanceHelper(Func<CancellationToken, Task> maintenanceTaskCreator, TimeSpan interval) | ||
| public MaintenanceHelper(Action<CancellationToken> maintenanceThreadCreator, TimeSpan interval) |
There was a problem hiding this comment.
I think a more accurate name would be maintenanceWorker or maintenanceAction or similar.
| @@ -76,7 +76,7 @@ internal sealed partial class ExclusiveConnectionPool : IConnectionPool | |||
| _connectionExceptionHandler = Ensure.IsNotNull(connectionExceptionHandler, nameof(connectionExceptionHandler)); | |||
| Ensure.IsNotNull(eventSubscriber, nameof(eventSubscriber)); | |||
|
|
|||
| _maintenanceHelper = new MaintenanceHelper(token => MaintainSizeAsync(token), _settings.MaintenanceInterval); | |||
| _maintenanceHelper = new MaintenanceHelper(token => MaintainSize(token), _settings.MaintenanceInterval); | |||
There was a problem hiding this comment.
minor: new MaintenanceHelper(MaintainSize, _settings.MaintenanceInterval);
| var maintenanceHelper = subject._maintenanceHelper(); | ||
| maintenanceHelper.Start(); | ||
| var maintenanceThread = ExclusiveConnectionPoolReflector._maintenanceThread(maintenanceHelper); | ||
| var isStopped = maintenanceThread // if this thread is completed first, it will mean that there was no delay (10 sec) |
There was a problem hiding this comment.
I am not sure how this test works. The thread will never stop unless the pool is paused. One idea would be to check that 2nd CreateConnection in called only after maintenanceInterval, or not called after x time (x << maintenanceInterval.
There was a problem hiding this comment.
This test had few issues, fixed
| @@ -1655,9 +1657,9 @@ private void InitializeAndWait(ExclusiveConnectionPool pool = null, ConnectionPo | |||
|
|
|||
| internal static class ExclusiveConnectionPoolReflector | |||
| { | |||
| public static Task MaintainSizeAsync(this ExclusiveConnectionPool obj, CancellationToken cancellationToken) | |||
| public static void MaintainSize(this ExclusiveConnectionPool obj, CancellationToken cancellationToken) | |||
There was a problem hiding this comment.
Do we still need this method?
| @@ -1145,7 +1145,7 @@ public void Maintenance_should_call_connection_dispose_when_connection_authentic | |||
| } | |||
|
|
|||
| [Fact] | |||
| public void MaintainSizeAsync_should_not_try_new_attempt_after_failing_without_delay() | |||
| public void MaintainSize_should_not_try_new_attempt_after_failing_without_delay() | |||
There was a problem hiding this comment.
I think it's a good time to add MaintenanceHelperTests, starting with simple start/stop test verifying thread creation and disposal.
There was a problem hiding this comment.
Maybe, but i would probably prefer to create a separate ticket for this
| @@ -164,17 +164,17 @@ public void ThrowIfNotInitialized() | |||
| internal sealed class MaintenanceHelper : IDisposable | |||
| { | |||
| private CancellationTokenSource _cancellationTokenSource = null; | |||
| private Func<CancellationToken, Task> _maintenanceTaskCreator; | |||
| private Task _maintenanceTask; | |||
| private Action<CancellationToken> _maintenanceAction; | |||
There was a problem hiding this comment.
minor: _maintenanceAction can be readonly.
| @@ -199,8 +200,13 @@ public void Start() | |||
| _cancellationTokenSource = new CancellationTokenSource(); | |||
| var cancellationToken = _cancellationTokenSource.Token; | |||
|
|
|||
| _maintenanceTask = Task.Run(() => _maintenanceTaskCreator(cancellationToken), cancellationToken); | |||
| _maintenanceTask.ConfigureAwait(false); | |||
| _maintenanceThread = new Thread(new ParameterizedThreadStart(ThreadStart)) { IsBackground = true }; | |||
There was a problem hiding this comment.
Additional point worth discussing, is whether it's better to destroy and create the thread every time, or just have single thread that sleeps when pool is paused.
No description provided.