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

Remove subsegments lock #278

Merged
merged 5 commits into from
Apr 22, 2021
Merged

Remove subsegments lock #278

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
79da46e
Remove subsegments lock
Apr 21, 2021
4e64d82
Deprecate getSubsegments
Apr 21, 2021
4cb99f2
Return a copy of subsegments list
Apr 21, 2021
70355e6
json-ignore
Apr 21, 2021
8e3eb90
Merge branch 'master' into remove-subsegments-lock
willarmiros Apr 22, 2021
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
5 changes: 5 additions & 0 deletions aws-xray-recorder-sdk-core/src/main/java/com/amazonaws/xray/entities/DummySegment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,11 @@ public List<Subsegment> getSubsegments() {
return list;
}

@Override
public List<Subsegment> getSubsegmentsCopy() {
return new ArrayList<>(list);
}

@Override
public void addSubsegment(Subsegment subsegment) {
}
Expand Down
5 changes: 5 additions & 0 deletions aws-xray-recorder-sdk-core/src/main/java/com/amazonaws/xray/entities/DummySubsegment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,11 @@ public List<Subsegment> getSubsegments() {
return list;
}

@Override
public List<Subsegment> getSubsegmentsCopy() {
return new ArrayList<>(list);
}

@Override
public void addSubsegment(Subsegment subsegment) {
}
Expand Down
11 changes: 10 additions & 1 deletion aws-xray-recorder-sdk-core/src/main/java/com/amazonaws/xray/entities/Entity.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ default void run(Runnable runnable, AWSXRayRecorder recorder) {
void setNamespace(String namespace);

/**
* @return the subsegmentsLock
* @return an unused {@link ReentrantLock}
*
* @deprecated This is for internal use of the SDK and will be made private.
*/
Expand Down Expand Up @@ -367,9 +367,18 @@ default void run(Runnable runnable, AWSXRayRecorder recorder) {

/**
* @return the subsegments
*
* @deprecated Use {@link #getSubsegmentsCopy()}.
*/
@Deprecated
List<Subsegment> getSubsegments();

/**
* Returns a copy of the currently added subsegments. Updates to the returned {@link List} will not be reflected in the
* {@link Entity}.
*/
List<Subsegment> getSubsegmentsCopy();

/**
* Adds a subsegment.
*
Expand Down
31 changes: 12 additions & 19 deletions aws-xray-recorder-sdk-core/src/main/java/com/amazonaws/xray/entities/EntityImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,16 +530,19 @@ public List<Subsegment> getSubsegments() {
}
}

@JsonIgnore
@Override
public List<Subsegment> getSubsegmentsCopy() {
synchronized (lock) {
return new ArrayList<>(subsegments);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we return an UnmodifiableCollection of the subsegments list here? Or do we intentionally want to return a snapshot of the current subsegments at copying time that is not updated with new subsegments?

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I think it is more clear, but won't mind if the proposed behavior is kept.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just wrapping the subsegments won't solve the thread safety possibility, it needs to be a copy. I could also wrap the copy with Unmodifiable but I don't think we really need to for copies.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok fair enough, yeah I guess doing an UnmodifiableCollection would still allow the user to mutate the individual subsegments.

}
}

@Override
public void addSubsegment(Subsegment subsegment) {
synchronized (lock) {
checkAlreadyEmitted();
getSubsegmentsLock().lock();
try {
subsegments.add(subsegment);
} finally {
getSubsegmentsLock().unlock();
}
subsegments.add(subsegment);
}
}

Expand All @@ -548,13 +551,8 @@ public void addException(Throwable exception) {
synchronized (lock) {
checkAlreadyEmitted();
setFault(true);
getSubsegmentsLock().lock();
try {
cause.addExceptions(creator.getThrowableSerializationStrategy()
.describeInContext(this, exception, subsegments));
} finally {
getSubsegmentsLock().unlock();
}
cause.addExceptions(creator.getThrowableSerializationStrategy()
.describeInContext(this, exception, subsegments));
}
}

Expand Down Expand Up @@ -756,12 +754,7 @@ public String prettySerialize() {
@Override
public void removeSubsegment(Subsegment subsegment) {
synchronized (lock) {
getSubsegmentsLock().lock();
try {
subsegments.remove(subsegment);
} finally {
getSubsegmentsLock().unlock();
}
subsegments.remove(subsegment);
getParentSegment().getTotalSize().decrement();
}
}
Expand Down
5 changes: 5 additions & 0 deletions aws-xray-recorder-sdk-core/src/main/java/com/amazonaws/xray/entities/NoOpSegment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -282,6 +282,11 @@ public List<Subsegment> getSubsegments() {
return NoOpList.get();
}

@Override
public List<Subsegment> getSubsegmentsCopy() {
return NoOpList.get();
}

@Override
public void addSubsegment(Subsegment subsegment) {
}
Expand Down
5 changes: 5 additions & 0 deletions aws-xray-recorder-sdk-core/src/main/java/com/amazonaws/xray/entities/NoOpSubSegment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,11 @@ public List<Subsegment> getSubsegments() {
return NoOpList.get();
}

@Override
public List<Subsegment> getSubsegmentsCopy() {
return NoOpList.get();
}

@Override
public void addSubsegment(Subsegment subsegment) {
}
Expand Down
23 changes: 6 additions & 17 deletions ...recorder-sdk-core/src/main/java/com/amazonaws/xray/strategy/DefaultStreamingStrategy.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
import com.amazonaws.xray.entities.Segment;
import com.amazonaws.xray.entities.Subsegment;
import java.util.ArrayList;
import java.util.List;

public class DefaultStreamingStrategy implements StreamingStrategy {

Expand Down Expand Up @@ -82,30 +83,18 @@ public boolean requiresStreaming(Segment segment) {
*/
@Override
public void streamSome(Entity entity, Emitter emitter) {
if (entity.getSubsegmentsLock().tryLock()) {
try {
stream(entity, emitter);
} finally {
entity.getSubsegmentsLock().unlock();
}
}
stream(entity, emitter);
}

private boolean stream(Entity entity, Emitter emitter) {
ArrayList<Subsegment> children = new ArrayList<>(entity.getSubsegments());
ArrayList<Subsegment> streamable = new ArrayList<>();
List<Subsegment> children = entity.getSubsegmentsCopy();
List<Subsegment> streamable = new ArrayList<>();

//Gather children and in the condition they are ready to stream, add them to the streamable list.
Copy link
Contributor

Choose a reason for hiding this comment

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

I think/fear a different race condition just moved 2 lines up now by removing the subsegmentLock. And you will hit #181 again.

As the copy of entity.getSubsegments() is taken outside the protected region.
So it is possible that an entity is added while the subsegment list is being copied what can/will result in a ConcurrentModificationException being thrown.

Possible solutions I see:

  • Make the collection returned by getSubsegments thread safe
  • Let getSubsegments return an (immutable) copy of the list (or provide another function which does).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for noticing this @steven-aerts - I've updated to return a copy

if (children.size() > 0) {
for (Subsegment child : children) {
if (child.getSubsegmentsLock().tryLock()) {
try {
if (stream(child, emitter)) {
streamable.add(child);
}
} finally {
child.getSubsegmentsLock().unlock();
}
if (stream(child, emitter)) {
streamable.add(child);
}
}
}
Expand Down