Skip to content

Commit f28042d

Browse files
authored
Fixed a bug in AttributeMap, which causes a ConcurrentModificationException… (#5559)
* Fixed a bug in AttributeMap, which causes a ConcurrentModificationException when lazy properties depend on lazy properties that depend on any other property. This is achieved by changing a `compute` into a `get` followed by a `put`. * Addressed PR comments.
1 parent 6c8c348 commit f28042d

File tree

2 files changed

+24
-11
lines changed

2 files changed

+24
-11
lines changed

utils/src/main/java/software/amazon/awssdk/utils/AttributeMap.java

Lines changed: 10 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -327,17 +327,17 @@ private void internalPut(Key<?> key, Value<?> value) {
327327
*/
328328
private Value<?> internalComputeIfAbsent(Key<?> key, Supplier<Value<?>> value) {
329329
checkCopyOnUpdate();
330-
return attributes.compute(key, (k, v) -> {
331-
if (v == null || resolveValue(v) == null) {
332-
Value<?> newValue = value.get();
333-
Validate.notNull(newValue, "Supplied value must not be null.");
334-
if (v != null) {
335-
dependencyGraph.valueUpdated(v, newValue);
336-
}
337-
return newValue;
330+
Value<?> currentValue = attributes.get(key);
331+
if (currentValue == null || resolveValue(currentValue) == null) {
332+
Value<?> newValue = value.get();
333+
Validate.notNull(newValue, "Supplied value must not be null.");
334+
if (currentValue != null) {
335+
dependencyGraph.valueUpdated(currentValue, newValue);
338336
}
339-
return v;
340-
});
337+
attributes.put(key, newValue);
338+
return newValue;
339+
}
340+
return currentValue;
341341
}
342342

343343
private void checkCopyOnUpdate() {

utils/src/test/java/software/amazon/awssdk/utils/AttributeMapTest.java

Lines changed: 14 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@
3030
import java.util.concurrent.TimeUnit;
3131
import org.junit.Test;
3232
import org.mockito.Mockito;
33-
import org.w3c.dom.Attr;
3433

3534
public class AttributeMapTest {
3635

@@ -158,6 +157,20 @@ public void lazyAttributes_notReResolvedAfterToBuilderBuild() {
158157
verify(lazyRead, Mockito.times(1)).run();
159158
}
160159

160+
@Test
161+
public void lazyAttributes_canUpdateTheMap_andBeUpdatedWithPutLazyIfAbsent() {
162+
AttributeMap.Builder map = AttributeMap.builder();
163+
map.putLazyIfAbsent(STRING_KEY, c -> {
164+
// Force a modification to the underlying map. We wouldn't usually do this so explicitly, but
165+
// this can happen internally within AttributeMap when resolving uncached lazy values,
166+
// so it needs to be possible.
167+
map.put(INTEGER_KEY, 0);
168+
return "string";
169+
});
170+
map.putLazyIfAbsent(STRING_KEY, c -> "string"); // Force the value to be resolved within the putLazyIfAbsent
171+
assertThat(map.get(STRING_KEY)).isEqualTo("string");
172+
}
173+
161174
@Test
162175
public void changesInBuilder_doNotAffectBuiltMap() {
163176
AttributeMap.Builder builder = mapBuilderWithLazyString();

0 commit comments

Comments
 (0)