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

Fix potential torn reads by counters #1073

Merged
merged 3 commits into from
Oct 13, 2020
Merged

Fix potential torn reads by counters #1073

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
32c101f
Fix potential torn reads by counters
JamesNK Oct 7, 2020
e89737b
Test
JamesNK Oct 8, 2020
ec805e3
Fix client event source
JamesNK Oct 8, 2020
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
14 changes: 7 additions & 7 deletions src/Grpc.AspNetCore.Server/Internal/GrpcEventSource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -136,31 +136,31 @@ protected override void OnEventCommand(EventCommandEventArgs command)
// This is the convention for initializing counters in the RuntimeEventSource (lazily on the first enable command).
// They aren't disabled afterwards...

_totalCallsCounter ??= new PollingCounter("total-calls", this, () => _totalCalls)
_totalCallsCounter ??= new PollingCounter("total-calls", this, () => Volatile.Read(ref _totalCalls))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's about Interlocked.Read vs. Volatile.Read.
In ASP.NET Core for the kestrel counters I've used Volatile.Read, which does the correct read, then opened dotnet/docs#20984 to update the docs. The consensus over there is to use Interlocked.Read as it's documented to prevent torn-reads.

I'm fine with either variant here, just wanted to point this fact out (and for completeness).

{
DisplayName = "Total Calls",
};
_currentCallsCounter ??= new PollingCounter("current-calls", this, () => _currentCalls)
_currentCallsCounter ??= new PollingCounter("current-calls", this, () => Volatile.Read(ref _currentCalls))
{
DisplayName = "Current Calls"
};
_callsFailedCounter ??= new PollingCounter("calls-failed", this, () => _callsFailed)
_callsFailedCounter ??= new PollingCounter("calls-failed", this, () => Volatile.Read(ref _callsFailed))
{
DisplayName = "Total Calls Failed",
};
_callsDeadlineExceededCounter ??= new PollingCounter("calls-deadline-exceeded", this, () => _callsDeadlineExceeded)
_callsDeadlineExceededCounter ??= new PollingCounter("calls-deadline-exceeded", this, () => Volatile.Read(ref _callsDeadlineExceeded))
{
DisplayName = "Total Calls Deadline Exceeded",
};
_messagesSentCounter ??= new PollingCounter("messages-sent", this, () => _messageSent)
_messagesSentCounter ??= new PollingCounter("messages-sent", this, () => Volatile.Read(ref _messageSent))
{
DisplayName = "Total Messages Sent",
};
_messagesReceivedCounter ??= new PollingCounter("messages-received", this, () => _messageReceived)
_messagesReceivedCounter ??= new PollingCounter("messages-received", this, () => Volatile.Read(ref _messageReceived))
{
DisplayName = "Total Messages Received",
};
_callsUnimplementedCounter ??= new PollingCounter("calls-unimplemented", this, () => _callsUnimplemented)
_callsUnimplementedCounter ??= new PollingCounter("calls-unimplemented", this, () => Volatile.Read(ref _callsUnimplemented))
{
DisplayName = "Total Calls Unimplemented",
};
Expand Down
12 changes: 6 additions & 6 deletions src/Grpc.Net.Client/Internal/GrpcEventSource.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,27 +125,27 @@ protected override void OnEventCommand(EventCommandEventArgs command)
// This is the convention for initializing counters in the RuntimeEventSource (lazily on the first enable command).
// They aren't disabled afterwards...

_totalCallsCounter ??= new PollingCounter("total-calls", this, () => _totalCalls)
_totalCallsCounter ??= new PollingCounter("total-calls", this, () => Volatile.Read(ref _totalCalls))
{
DisplayName = "Total Calls",
};
_currentCallsCounter ??= new PollingCounter("current-calls", this, () => _currentCalls)
_currentCallsCounter ??= new PollingCounter("current-calls", this, () => Volatile.Read(ref _currentCalls))
{
DisplayName = "Current Calls"
};
_callsFailedCounter ??= new PollingCounter("calls-failed", this, () => _callsFailed)
_callsFailedCounter ??= new PollingCounter("calls-failed", this, () => Volatile.Read(ref _callsFailed))
{
DisplayName = "Total Calls Failed",
};
_callsDeadlineExceededCounter ??= new PollingCounter("calls-deadline-exceeded", this, () => _callsDeadlineExceeded)
_callsDeadlineExceededCounter ??= new PollingCounter("calls-deadline-exceeded", this, () => Volatile.Read(ref _callsDeadlineExceeded))
{
DisplayName = "Total Calls Deadline Exceeded",
};
_messagesSentCounter ??= new PollingCounter("messages-sent", this, () => _messageSent)
_messagesSentCounter ??= new PollingCounter("messages-sent", this, () => Volatile.Read(ref _messageSent))
{
DisplayName = "Total Messages Sent",
};
_messagesReceivedCounter ??= new PollingCounter("messages-received", this, () => _messageReceived)
_messagesReceivedCounter ??= new PollingCounter("messages-received", this, () => Volatile.Read(ref _messageReceived))
{
DisplayName = "Total Messages Received",
};
Expand Down
4 changes: 4 additions & 0 deletions test/FunctionalTests/Client/CancellationTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,10 @@ await call.RequestStream.WriteAsync(new DataMessage
throw;
}
});

// Wait a short amount of time so that any server cancellation error
// finishes being thrown before the next test starts.
await Task.Delay(50);
}

[Test]
Expand Down