-
Notifications
You must be signed in to change notification settings - Fork 923
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
some TraceableSession functions do not await the activity #2294
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #2294 +/- ##
==========================================
+ Coverage 58.84% 59.07% +0.23%
==========================================
Files 330 330
Lines 63320 63320
==========================================
+ Hits 37262 37408 +146
+ Misses 26058 25912 -146
☔ View full report in Codecov by Sentry. |
@@ -390,11 +390,11 @@ public ReferenceDescription FindDataDescription(NodeId encodingId) | |||
} | |||
|
|||
/// <inheritdoc/> | |||
public Task<DataDictionary> FindDataDictionary(NodeId descriptionId) | |||
public async Task<DataDictionary> FindDataDictionary(NodeId descriptionId) |
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.
Also, if the method returns a Task, it needs to have the suffix Async: FindDataDictionaryAsync
@@ -956,11 +956,11 @@ public ResponseHeader EndAddNodes(IAsyncResult result, out AddNodesResultCollect | |||
} | |||
|
|||
/// <inheritdoc/> | |||
public Task<AddNodesResponse> AddNodesAsync(RequestHeader requestHeader, AddNodesItemCollection nodesToAdd, CancellationToken ct) | |||
public async Task<AddNodesResponse> AddNodesAsync(RequestHeader requestHeader, AddNodesItemCollection nodesToAdd, CancellationToken ct) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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 retract my comment. When using "using", then this is a must.
@@ -390,11 +390,11 @@ public ReferenceDescription FindDataDescription(NodeId encodingId) | |||
} | |||
|
|||
/// <inheritdoc/> | |||
public Task<DataDictionary> FindDataDictionary(NodeId descriptionId) | |||
public async Task<DataDictionary> FindDataDictionary(NodeId descriptionId) |
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.
Ah, I retract my comment. When using "using", then this is a must.
Proposed changes
Current Behavior
e.g. this call doesn't measure the duraction correctly because the Task returns directly, the caller waits
public Task TransferSubscriptionsAsync(SubscriptionCollection subscriptions, bool sendInitialValues, CancellationToken ct = default)
{
using (Activity activity = ActivitySource.StartActivity(nameof(TransferSubscriptionsAsync)))
{
return m_session.TransferSubscriptionsAsync(subscriptions, sendInitialValues, ct);
}
}
public async Task TransferSubscriptionsAsync(SubscriptionCollection subscriptions, bool sendInitialValues, CancellationToken ct = default)
{
using (Activity activity = ActivitySource.StartActivity(nameof(TransferSubscriptionsAsync)))
{
return await m_session.TransferSubscriptionsAsync(subscriptions, sendInitialValues, ct).ConfigureAwait(false);
}
}
Related Issues
some TraceableSession functions do not await the activity #2293
Types of changes
What types of changes does your code introduce?
Put an
x
in the boxes that apply. You can also fill these out after creating the PR.Checklist
Put an
x
in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.Further comments
If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...