Lock all accesses to entities. #250
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
willarmiros
reviewed
Jan 20, 2021
| } finally { | ||
| getSubsegmentsLock().unlock(); | ||
| synchronized (lock) { | ||
| getSubsegmentsLock().lock(); |
There was a problem hiding this comment.
Do we still need to lock the subsegments list when we're locking on the entire parent entity?
willarmiros
approved these changes
Jan 20, 2021
There was a problem hiding this comment.
LGTM, we'll need to keep an eye on benchmarks before next release but shouldn't be a big deal. My guess is this should resolve #108 since the issue there was that during serialization, a subsegment could be removed from the entity being serialized. Since the serialize method and the methods to add/remove subsegments are controlled by the same lock now, it should be ok now. But can't be sure without reliable repro case/test.
willarmiros
pushed a commit
to willarmiros/aws-xray-sdk-java
that referenced
this pull request
Aug 13, 2022
This reverts commit f0a3b3a.
wangzlei
pushed a commit
that referenced
this pull request
Aug 22, 2022
* Revert "Don't lock segment from subsegment (#306)" This reverts commit d5ccae2. * Revert "Keep track of emitted in emitter. (#279)" This reverts commit d66d69a. * Revert "Remove subsegments lock (#278)" This reverts commit adfb395. * Revert "Lock all accesses to entities. (#250)" This reverts commit f0a3b3a. * add back errorprone annotations * fixed shouldPropagate * added back getSubsegmentsCopy * added back compareAndSetEmitted * whitespace fix
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue #, if available: #108
Description of changes:
Almost none of the entity accesses are currently safe since there are many mutations of non-volatile fields. While making fields volatile is one way to help, modern CPUs have fast enough performance for non-contended locks (just atomic CAS) that it's common to just use locks (Golang makes it very hard to use volatile for this reason). This is the implementation OTel uses
https://github.com/open-telemetry/opentelemetry-java/blob/master/sdk/trace/src/main/java/io/opentelemetry/sdk/trace/RecordEventsReadableSpan.java
OTel doesn't have setters for every single thing so doesn't need as much locking, but as we don't expect much contention here it should be fine.
I haven't quite reasoned through whether this fixes the subsegment issues, but it's an important change regardless.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.