Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Ensure That Timer Callback Does not Call Method on Disposed Timer #1445

Merged
merged 3 commits into from
Oct 11, 2021
Merged

Ensure That Timer Callback Does not Call Method on Disposed Timer #1445

Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6ff1047
ensure that timer callback does not call method on disposed timer
thefringeninja Oct 6, 2021
64f34f1
Add locking to deadline timer disposal
JamesNK Oct 6, 2021
dd9bd36
Remove null check from client's deadline timer call
JamesNK Oct 8, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 32 additions & 12 deletions src/Grpc.Net.Client/Internal/GrpcCall.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,7 +225,18 @@ private void Cleanup(Status status)
Channel.FinishActiveCall(this);

_ctsRegistration?.Dispose();
_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();
Expand Down Expand Up @@ -650,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;
Expand Down Expand Up @@ -769,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();
Expand Down Expand Up @@ -812,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;
}

Expand Down Expand Up @@ -958,14 +975,17 @@ 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);
}
}
}
}

Expand Down