Remove subsegments lock #278
Conversation
| @@ -98,14 +92,8 @@ private boolean stream(Entity entity, Emitter emitter) { | |||
| //Gather children and in the condition they are ready to stream, add them to the streamable list. | |||
There was a problem hiding this comment.
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
getSubsegmentsthread safe - Let
getSubsegmentsreturn an (immutable) copy of the list (or provide another function which does).
There was a problem hiding this comment.
Thanks for noticing this @steven-aerts - I've updated to return a copy
There was a problem hiding this comment.
I wonder if we can cook up a test to try to reproduce #276? I'm imagining something with 2 threads, forcing the deadlock with Thread.sleep where needed. If it's too flaky or too much work that's ok, but would be nice to have.
| @Override | ||
| public List<Subsegment> getSubsegmentsCopy() { | ||
| synchronized (lock) { | ||
| return new ArrayList<>(subsegments); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Personally I think it is more clear, but won't mind if the proposed behavior is kept.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok fair enough, yeah I guess doing an UnmodifiableCollection would still allow the user to mutate the individual subsegments.
There was a problem hiding this comment.
I wonder if we can cook up a test to try to reproduce #276? I'm imagining something with 2 threads, forcing the deadlock with
Thread.sleepwhere needed. If it's too flaky or too much work that's ok, but would be nice to have.
@willarmiros I tried when I created this ticket but was not able to do so. Only saw it in live code :-( .
| @Override | ||
| public List<Subsegment> getSubsegmentsCopy() { | ||
| synchronized (lock) { | ||
| return new ArrayList<>(subsegments); |
There was a problem hiding this comment.
Personally I think it is more clear, but won't mind if the proposed behavior is kept.
This reverts commit adfb395.
* 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
Issue #, if available: Fixes #276
Description of changes:
Since we lock all access to the segment now, the subsegments lock is mostly not necessary. The only pattern it was needed for is user code doing something like
But it's basically impossible to make code deadlock-free when returning locks to unknown code and is basically a forbidden practice in Java. I think it's best we just hope no one is doing this, and if someone is and tells us, we recommend the unfortunate code change of switching to
addSubsegment.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.