Make AccumulatingStagingArea Thread Safe#2288
Conversation
| Queue<StackTrace> stackTraces = this.stackTraces.remove(traceId); | ||
| if (stackTraces != null) { | ||
| exporter.get().export(stackTraces); | ||
| exporter.get().export(new ArrayList<>(stackTraces)); |
There was a problem hiding this comment.
if you made the StackTraceExporter accept a Collection than you could skip the copy.
| (id, stackTraces) -> { | ||
| if (stackTraces == null) { | ||
| stackTraces = new ArrayList<>(); | ||
| stackTraces = new ConcurrentLinkedQueue<>(); |
There was a problem hiding this comment.
When empty is called before the last stage from the schedule then you will end up leaking a map entry.
There was a problem hiding this comment.
@laurit yes, though not a thread-safety issue and not introduced by this PR.
There was a problem hiding this comment.
I see two issues with this class. Agree with Lauri that the leaking of map objects is an issue, as is needing to access 2 concurrent datastructures to add samples. Wouldn't we be better off using ScheduledExecutorStackTraceSampler as a stagingArea since that is already per Thread and would solve above two issues?
If there is a concern in future we move to multi-threaded stack traces then the solution should be implemented in a way that won't make calling stage so expensive.
@lo-jason The primary purpose of this PR is to both address concerns raised in #2277 and #2262 without the scope of those PRs growing beyond the specific goal of each. In both of those PRs the thread-safety problems of Further #2262 is preventing the replacement of I'm reluctant to expend too much effort perfecting this implementation when I intend to immediately replace it (pending the merging of #2262) |
3 participants
Stores staged
StackTraceinstances in a thread-safeQueuewhile exporting a "view" of thatQueuewhenemptyis called.