From 5901f86da51d9224a2b6954823af70ed530593da Mon Sep 17 00:00:00 2001 From: Dhananjay Badaya <11253243+dbadaya1@users.noreply.github.com> Date: Thu, 9 Dec 2021 12:55:02 +0530 Subject: [PATCH 1/2] HADOOP-13500. Synchronizing iteration of underlying properties object in Configuration class It is possible to encounter a ConcurrentModificationException while trying to iterate a Configuration object. The iterator method tries to walk the underlying Property object without proper synchronization, so another thread simultaneously calling the set method can trigger it. setProperty method on Property object is also synchronized on the same property object. --- .../org/apache/hadoop/conf/Configuration.java | 10 ++++--- .../apache/hadoop/conf/TestConfiguration.java | 29 +++++++++++++++++++ 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java index 28be4beb51eb1..d784f3b6c7af6 100755 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java @@ -2978,11 +2978,13 @@ public Iterator> iterator() { // methods that allow non-strings to be put into configurations are removed, // we could replace properties with a Map and get rid of this // code. - Map result = new HashMap(); - for(Map.Entry item: getProps().entrySet()) { - if (item.getKey() instanceof String && - item.getValue() instanceof String) { + Properties properties = getProps(); + Map result = new HashMap<>(); + synchronized (properties) { + for (Map.Entry item : properties.entrySet()) { + if (item.getKey() instanceof String && item.getValue() instanceof String) { result.put((String) item.getKey(), (String) item.getValue()); + } } } return result.entrySet().iterator(); diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java index 731fbc430b746..fa6f05d59db45 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java @@ -38,6 +38,7 @@ import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.ConcurrentModificationException; import java.util.HashMap; import java.util.HashSet; import java.util.List; @@ -45,6 +46,7 @@ import java.util.Properties; import java.util.Random; import java.util.Set; +import java.util.concurrent.atomic.AtomicBoolean; import java.util.regex.Pattern; import static java.util.concurrent.TimeUnit.*; @@ -2687,4 +2689,31 @@ private static Configuration checkCDATA(byte[] bytes) { assertEquals(" prefix >cdata\nsuffix ", conf.get("cdata-whitespace")); return conf; } + + @Test + public void testConcurrentModificationDuringIteration() throws InterruptedException { + Configuration conf = new Configuration(); + new Thread(() -> { + while (true) { + conf.set(String.valueOf(Math.random()), String.valueOf(Math.random())); + } + }).start(); + + AtomicBoolean exceptionOccurred = new AtomicBoolean(false); + + new Thread(() -> { + while (true) { + try { + conf.iterator(); + } catch (final ConcurrentModificationException e) { + exceptionOccurred.set(true); + break; + } + } + }).start(); + + Thread.sleep(1000); //give enough time for threads to run + + assertFalse("ConcurrentModificationException occurred", exceptionOccurred.get()); + } } From 275ce3544ded13916f0cf88a7f8096fe86bbddb3 Mon Sep 17 00:00:00 2001 From: Dhananjay Badaya <11253243+dbadaya1@users.noreply.github.com> Date: Thu, 16 Dec 2021 17:24:39 +0530 Subject: [PATCH 2/2] Fix checkstyle issues --- .../src/main/java/org/apache/hadoop/conf/Configuration.java | 6 +++--- .../test/java/org/apache/hadoop/conf/TestConfiguration.java | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java index d784f3b6c7af6..1f809b7b54706 100755 --- a/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java +++ b/hadoop-common-project/hadoop-common/src/main/java/org/apache/hadoop/conf/Configuration.java @@ -2978,10 +2978,10 @@ public Iterator> iterator() { // methods that allow non-strings to be put into configurations are removed, // we could replace properties with a Map and get rid of this // code. - Properties properties = getProps(); + Properties props = getProps(); Map result = new HashMap<>(); - synchronized (properties) { - for (Map.Entry item : properties.entrySet()) { + synchronized (props) { + for (Map.Entry item : props.entrySet()) { if (item.getKey() instanceof String && item.getValue() instanceof String) { result.put((String) item.getKey(), (String) item.getValue()); } diff --git a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java index fa6f05d59db45..b3487ef309fc9 100644 --- a/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java +++ b/hadoop-common-project/hadoop-common/src/test/java/org/apache/hadoop/conf/TestConfiguration.java @@ -2692,10 +2692,10 @@ private static Configuration checkCDATA(byte[] bytes) { @Test public void testConcurrentModificationDuringIteration() throws InterruptedException { - Configuration conf = new Configuration(); + Configuration configuration = new Configuration(); new Thread(() -> { while (true) { - conf.set(String.valueOf(Math.random()), String.valueOf(Math.random())); + configuration.set(String.valueOf(Math.random()), String.valueOf(Math.random())); } }).start(); @@ -2704,7 +2704,7 @@ public void testConcurrentModificationDuringIteration() throws InterruptedExcept new Thread(() -> { while (true) { try { - conf.iterator(); + configuration.iterator(); } catch (final ConcurrentModificationException e) { exceptionOccurred.set(true); break;