Batching activity processor #755
Conversation
| { | ||
| if (!this.stopping) | ||
| { | ||
| this.ShutdownAsync(CancellationToken.None).ContinueWith(_ => { }).GetAwaiter().GetResult(); |
There was a problem hiding this comment.
This could introduce exception which is against the .NET guideline: The object must not throw an exception if its Dispose method is called multiple times.
Reason: ShutdownAsync is not thread safe, race condition could happen for this.stopping, which would re-enter the following code with this.cts == null:
this.cts.Cancel(false);
this.cts.Dispose();
this.cts = null;
There was a problem hiding this comment.
What's with ContinueWith(_ => { }) is that to suppress any exception?
There was a problem hiding this comment.
Yes. This will suppress an exception.
There was a problem hiding this comment.
@reyang @CodeBlanch @cijothomas
We have similar dispose pattern in SimpleSpanProcessor, SimpleActivityProcessor, BatchingSpanProcessor. Should we create a new issue to address it? or we consider fixing as a part of this PR?
There was a problem hiding this comment.
i'd open a new issue and merge this one. So that we can make progress and keep track of fixing the underlying issue.
There was a problem hiding this comment.
Created an issue #769 to track it.
Changes
Adds a batching activity processor.
Added exporterTimeout #578
As #697 is inactive, created this new PR to add batching activity processor and fixed tests
Checklist