From 6ff10472c3066a2cffbe279ffd92fdee5437e5d1 Mon Sep 17 00:00:00 2001 From: thefringeninja <495495+thefringeninja@users.noreply.github.com> Date: Wed, 6 Oct 2021 16:02:07 +0200 Subject: [PATCH 1/3] ensure that timer callback does not call method on disposed timer --- src/Grpc.Net.Client/Internal/GrpcCall.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/Grpc.Net.Client/Internal/GrpcCall.cs b/src/Grpc.Net.Client/Internal/GrpcCall.cs index a50126fb1..40d085595 100644 --- a/src/Grpc.Net.Client/Internal/GrpcCall.cs +++ b/src/Grpc.Net.Client/Internal/GrpcCall.cs @@ -225,7 +225,9 @@ private void Cleanup(Status status) Channel.FinishActiveCall(this); _ctsRegistration?.Dispose(); - _deadlineTimer?.Dispose(); + var deadlineTimer = _deadlineTimer; + _deadlineTimer = null; + deadlineTimer?.Dispose(); HttpResponse?.Dispose(); ClientStreamReader?.Dispose(); ClientStreamWriter?.Dispose(); @@ -965,7 +967,7 @@ private void DeadlineExceededCallback(object? state) GrpcCallLog.DeadlineTimerRescheduled(Logger, remaining); var dueTime = CommonGrpcProtocolHelpers.GetTimerDueTime(remaining, Channel.MaxTimerDueTime); - _deadlineTimer!.Change(dueTime, Timeout.Infinite); + _deadlineTimer?.Change(dueTime, Timeout.Infinite); } } From 64f34f19201adabfecfc74fad20fa8d70fa27fd0 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Thu, 7 Oct 2021 08:45:00 +1300 Subject: [PATCH 2/3] Add locking to deadline timer disposal --- src/Grpc.Net.Client/Internal/GrpcCall.cs | 47 +++++++++++++++++------- 1 file changed, 33 insertions(+), 14 deletions(-) diff --git a/src/Grpc.Net.Client/Internal/GrpcCall.cs b/src/Grpc.Net.Client/Internal/GrpcCall.cs index 40d085595..941706d94 100644 --- a/src/Grpc.Net.Client/Internal/GrpcCall.cs +++ b/src/Grpc.Net.Client/Internal/GrpcCall.cs @@ -225,9 +225,18 @@ private void Cleanup(Status status) Channel.FinishActiveCall(this); _ctsRegistration?.Dispose(); - var deadlineTimer = _deadlineTimer; - _deadlineTimer = null; - deadlineTimer?.Dispose(); + + if (_deadlineTimer != null) + { + lock (this) + { + // Timer callback can call Timer.Change so dispose deadline timer in a lock + // and set to null to indicate to the callback that it has been disposed. + _deadlineTimer?.Dispose(); + _deadlineTimer = null; + } + } + HttpResponse?.Dispose(); ClientStreamReader?.Dispose(); ClientStreamWriter?.Dispose(); @@ -652,10 +661,15 @@ internal bool ResolveException(string summary, Exception ex, [NotNull] out Statu // The server could exceed the deadline and return a CANCELLED status before the // client's deadline timer is triggered. When CANCELLED is received check the // deadline against the clock and change status to DEADLINE_EXCEEDED if required. - var deadlineExceeded = s.StatusCode == StatusCode.Cancelled && IsDeadlineExceeded(); - if (deadlineExceeded) + if (s.StatusCode == StatusCode.Cancelled) { - s = new Status(StatusCode.DeadlineExceeded, s.Detail, s.DebugException); + lock (this) + { + if (IsDeadlineExceededUnsynchronized()) + { + s = new Status(StatusCode.DeadlineExceeded, s.Detail, s.DebugException); + } + } } status = s; @@ -771,7 +785,7 @@ private bool FinishCall(HttpRequestMessage request, bool diagnosticSourceEnabled // the client has processed that it has exceeded or not. lock (this) { - if (IsDeadlineExceeded()) + if (IsDeadlineExceededUnsynchronized()) { GrpcCallLog.DeadlineExceeded(Logger); GrpcEventSource.Log.CallDeadlineExceeded(); @@ -814,8 +828,9 @@ private bool FinishCall(HttpRequestMessage request, bool diagnosticSourceEnabled return true; } - private bool IsDeadlineExceeded() + private bool IsDeadlineExceededUnsynchronized() { + Debug.Assert(Monitor.IsEntered(this), "Check deadline in a lock. Updating a DateTime isn't guaranteed to be atomic. Avoid struct tearing."); return _deadline <= Channel.Clock.UtcNow; } @@ -960,14 +975,18 @@ private void DeadlineExceededCallback(object? state) DeadlineExceeded(); return; } - } - // Deadline has not been reached because timer maximum due time was smaller than deadline. - // Reschedule DeadlineExceeded again until deadline has been exceeded. - GrpcCallLog.DeadlineTimerRescheduled(Logger, remaining); + if (_deadlineTimer != null) + { + // Deadline has not been reached because timer maximum due time was smaller than deadline. + // Reschedule DeadlineExceeded again until deadline has been exceeded. + GrpcCallLog.DeadlineTimerRescheduled(Logger, remaining); + + var dueTime = CommonGrpcProtocolHelpers.GetTimerDueTime(remaining, Channel.MaxTimerDueTime); + _deadlineTimer?.Change(dueTime, Timeout.Infinite); - var dueTime = CommonGrpcProtocolHelpers.GetTimerDueTime(remaining, Channel.MaxTimerDueTime); - _deadlineTimer?.Change(dueTime, Timeout.Infinite); + } + } } } From dd9bd36c1636cd0dade986421278bc703a395fa1 Mon Sep 17 00:00:00 2001 From: James Newton-King Date: Sat, 9 Oct 2021 11:29:45 +1300 Subject: [PATCH 3/3] Remove null check from client's deadline timer call --- src/Grpc.Net.Client/Internal/GrpcCall.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/Grpc.Net.Client/Internal/GrpcCall.cs b/src/Grpc.Net.Client/Internal/GrpcCall.cs index 941706d94..80bb6a49d 100644 --- a/src/Grpc.Net.Client/Internal/GrpcCall.cs +++ b/src/Grpc.Net.Client/Internal/GrpcCall.cs @@ -983,8 +983,7 @@ private void DeadlineExceededCallback(object? state) GrpcCallLog.DeadlineTimerRescheduled(Logger, remaining); var dueTime = CommonGrpcProtocolHelpers.GetTimerDueTime(remaining, Channel.MaxTimerDueTime); - _deadlineTimer?.Change(dueTime, Timeout.Infinite); - + _deadlineTimer.Change(dueTime, Timeout.Infinite); } } }