-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
CSHARP-3906: Switch ConnectionPool background task to a dedicated thread. #739
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
{ | ||
_maintenanceThreadCreator((CancellationToken)cancellationToken); | ||
} | ||
catch (OperationCanceledException) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need OperationCanceledException
here, let it be maintenanceAction
responsibility (it is already).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, done
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a more accurate name would be maintenanceWorker
or maintenanceAction
or similar.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor: new MaintenanceHelper(MaintainSize, _settings.MaintenanceInterval);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we still need this method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
now it's needed
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe, but i would probably prefer to create a separate ticket for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM (minor comment left)
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.