From 21b7f899ab3d995a8f55245242a0393b66805b45 Mon Sep 17 00:00:00 2001 From: yawkat Date: Tue, 10 Dec 2024 14:43:45 +0100 Subject: [PATCH 1/8] Avoid type pollution in StringCollectionDeserializer This patch fixes a type pollution issue when deserializing a `List` property, by adding explicit type checks for common subclasses (ArrayList, HashSet). The type pollution issue is a Hotspot performance problem prior to OpenJDK 23, where repeated type checks of the same concrete type against multiple interface types can lead to significant performance problems in multi-core systems. The issue is described in detail [in this blog post](https://redhatperf.github.io/post/type-check-scalability-issue/) from the finders at RedHat, and I've also written up a basic description [in the micronaut-test documentation](https://micronaut-projects.github.io/micronaut-test/latest/guide/#typePollution). In jackson-databind, this issue can manifest when deserializing a `List` property. The property will be deserialized as ArrayList. StringCollectionDeserializer will then cast this ArrayList to Collection, causing a type check. Later on, the BeanPropertyWriter will set this field (or call the constructor, or call the setter method), causing a second type check against the declared type of the property, List. This back-and-forth can trigger the JDK bug. Replacing the Collection cast by an ArrayList cast when possible fixes this issue. Attributing a production performance issue to type pollution is tricky, so the RedHat folks developed [a Java agent](https://github.com/RedHatPerf/type-pollution-agent) that instruments all the bytecode of an application, tracks type checks, and reports any potential type pollution. In micronaut-test, I've adapted this approach to make it workable in unit tests. micronaut-test-type-pollution does not require use of other parts of the micronaut framework. In this PR, on top of the actual fix, I've included a test based on micronaut-test-type-pollution. This test runs as a separate test execution because it does not play well with other agents (in particular jacoco). I use junit 5 test suites for this. This test protects us against regressions, since they can happen when modifying seemingly unrelated code, and also gives others the option to test more complex structures for similar issues. Another option is to just not test this, which would remove the need for the build changes. --- pom.xml | 45 ++++++++- .../std/StringCollectionDeserializer.java | 55 +++++++---- .../typepollution/TypePollutionSuite.java | 9 ++ .../typepollution/TypePollutionTest.java | 95 +++++++++++++++++++ .../jackson/databind/PrimarySuite.java | 16 ++++ 5 files changed, 197 insertions(+), 23 deletions(-) create mode 100644 src/test-jdk17/java/com/fasterxml/jackson/databind/typepollution/TypePollutionSuite.java create mode 100644 src/test-jdk17/java/com/fasterxml/jackson/databind/typepollution/TypePollutionTest.java create mode 100644 src/test/java/com/fasterxml/jackson/databind/PrimarySuite.java diff --git a/pom.xml b/pom.xml index 2e93f3cfb2..867771ea2f 100644 --- a/pom.xml +++ b/pom.xml @@ -115,6 +115,12 @@ junit-jupiter-api test + + org.junit.platform + junit-platform-suite-engine + 1.10.2 + test + org.assertj assertj-core @@ -140,6 +146,12 @@ 0.16 test + + io.micronaut.test + micronaut-test-type-pollution + 4.6.2 + test + @@ -210,10 +222,7 @@ javax.measure:jsr-275 - - com.fasterxml.jackson.databind.MapperFootprintTest - **/failing/**/*.java - + com.fasterxml.jackson.databind.PrimarySuite @@ -387,6 +396,19 @@ --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED + + + type-pollution-test + test + + test + + + com.fasterxml.jackson.databind.typepollution.TypePollutionSuite + 1 + + + @@ -438,8 +460,21 @@ org.apache.maven.plugins maven-surefire-plugin - --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED + --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED -XX:+EnableDynamicAgentLoading + + + type-pollution-test + test + + test + + + com.fasterxml.jackson.databind.typepollution.TypePollutionSuite + 1 + + + diff --git a/src/main/java/com/fasterxml/jackson/databind/deser/std/StringCollectionDeserializer.java b/src/main/java/com/fasterxml/jackson/databind/deser/std/StringCollectionDeserializer.java index 8997265021..78d844302e 100644 --- a/src/main/java/com/fasterxml/jackson/databind/deser/std/StringCollectionDeserializer.java +++ b/src/main/java/com/fasterxml/jackson/databind/deser/std/StringCollectionDeserializer.java @@ -1,14 +1,14 @@ package com.fasterxml.jackson.databind.deser.std; -import java.io.IOException; -import java.util.Collection; -import java.util.Objects; - import com.fasterxml.jackson.annotation.JsonFormat; - -import com.fasterxml.jackson.core.*; - -import com.fasterxml.jackson.databind.*; +import com.fasterxml.jackson.core.JsonParser; +import com.fasterxml.jackson.core.JsonToken; +import com.fasterxml.jackson.databind.BeanProperty; +import com.fasterxml.jackson.databind.DeserializationContext; +import com.fasterxml.jackson.databind.DeserializationFeature; +import com.fasterxml.jackson.databind.JavaType; +import com.fasterxml.jackson.databind.JsonDeserializer; +import com.fasterxml.jackson.databind.JsonMappingException; import com.fasterxml.jackson.databind.annotation.JacksonStdImpl; import com.fasterxml.jackson.databind.cfg.CoercionAction; import com.fasterxml.jackson.databind.cfg.CoercionInputShape; @@ -19,6 +19,12 @@ import com.fasterxml.jackson.databind.jsontype.TypeDeserializer; import com.fasterxml.jackson.databind.type.LogicalType; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collection; +import java.util.HashSet; +import java.util.Objects; + /** * Specifically optimized version for {@link java.util.Collection}s * that contain String values; reason is that this is a very common @@ -171,16 +177,15 @@ public ValueInstantiator getValueInstantiator() { /********************************************************** */ - @SuppressWarnings("unchecked") @Override public Collection deserialize(JsonParser p, DeserializationContext ctxt) throws IOException { if (_delegateDeserializer != null) { - return (Collection) _valueInstantiator.createUsingDelegate(ctxt, - _delegateDeserializer.deserialize(p, ctxt)); + return castToCollection(_valueInstantiator.createUsingDelegate(ctxt, + _delegateDeserializer.deserialize(p, ctxt))); } - final Collection result = (Collection) _valueInstantiator.createUsingDefault(ctxt); + final Collection result = castToCollection(_valueInstantiator.createUsingDefault(ctxt)); return deserialize(p, ctxt, result); } @@ -273,7 +278,6 @@ public Object deserializeWithType(JsonParser p, DeserializationContext ctxt, * throw an exception, or try to handle value as if member of implicit * array, depending on configuration. */ - @SuppressWarnings("unchecked") private final Collection handleNonArray(JsonParser p, DeserializationContext ctxt, Collection result) throws IOException { @@ -285,7 +289,7 @@ private final Collection handleNonArray(JsonParser p, DeserializationCon if (p.hasToken(JsonToken.VALUE_STRING)) { return _deserializeFromString(p, ctxt); } - return (Collection) ctxt.handleUnexpectedToken(_containerType, p); + return castToCollection(ctxt.handleUnexpectedToken(_containerType, p)); } // Strings are one of "native" (intrinsic) types, so there's never type deserializer involved JsonDeserializer valueDes = _valueDeserializer; @@ -307,15 +311,15 @@ private final Collection handleNonArray(JsonParser p, DeserializationCon final CoercionAction act = ctxt.findCoercionAction(logicalType(), handledType(), CoercionInputShape.EmptyString); if (act != CoercionAction.Fail) { - return (Collection) _deserializeFromEmptyString(p, ctxt, act, handledType(), - "empty String (\"\")"); + return castToCollection(_deserializeFromEmptyString(p, ctxt, act, handledType(), + "empty String (\"\")")); } } else if (_isBlank(textValue)) { final CoercionAction act = ctxt.findCoercionFromBlankString(logicalType(), handledType(), CoercionAction.Fail); if (act != CoercionAction.Fail) { - return (Collection) _deserializeFromEmptyString(p, ctxt, act, handledType(), - "blank String (all whitespace)"); + return castToCollection(_deserializeFromEmptyString(p, ctxt, act, handledType(), + "blank String (all whitespace)")); } } // if coercion failed, we can still add it to a list @@ -330,4 +334,19 @@ private final Collection handleNonArray(JsonParser p, DeserializationCon result.add(value); return result; } + + @SuppressWarnings("unchecked") + private static Collection castToCollection(Object o) { + if (o != null) { + // fast path for specific classes to avoid type pollution: + // https://micronaut-projects.github.io/micronaut-test/latest/guide/#typePollution + if (o.getClass() == ArrayList.class) { + return (ArrayList) o; + } + if (o.getClass() == HashSet.class) { + return (HashSet) o; + } + } + return (Collection) o; + } } diff --git a/src/test-jdk17/java/com/fasterxml/jackson/databind/typepollution/TypePollutionSuite.java b/src/test-jdk17/java/com/fasterxml/jackson/databind/typepollution/TypePollutionSuite.java new file mode 100644 index 0000000000..beb05ab08e --- /dev/null +++ b/src/test-jdk17/java/com/fasterxml/jackson/databind/typepollution/TypePollutionSuite.java @@ -0,0 +1,9 @@ +package com.fasterxml.jackson.databind.typepollution; + +import org.junit.platform.suite.api.SelectPackages; +import org.junit.platform.suite.api.Suite; + +@Suite +@SelectPackages("com.fasterxml.jackson.databind.typepollution") +public class TypePollutionSuite { +} diff --git a/src/test-jdk17/java/com/fasterxml/jackson/databind/typepollution/TypePollutionTest.java b/src/test-jdk17/java/com/fasterxml/jackson/databind/typepollution/TypePollutionTest.java new file mode 100644 index 0000000000..4e1a2b5d70 --- /dev/null +++ b/src/test-jdk17/java/com/fasterxml/jackson/databind/typepollution/TypePollutionTest.java @@ -0,0 +1,95 @@ +package com.fasterxml.jackson.databind.typepollution; + +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.json.JsonMapper; +import io.micronaut.test.typepollution.FocusListener; +import io.micronaut.test.typepollution.ThresholdFocusListener; +import io.micronaut.test.typepollution.TypePollutionTransformer; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Disabled; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.function.Executable; + +import java.util.List; +import java.util.Set; +import java.util.concurrent.atomic.AtomicInteger; + +public class TypePollutionTest { + private static final int THRESHOLD = 1000; + private ThresholdFocusListener focusListener; + + @BeforeAll + static void setupAgent() { + TypePollutionTransformer.install(net.bytebuddy.agent.ByteBuddyAgent.install()); + } + + @BeforeEach + void setUp() { + focusListener = new ThresholdFocusListener(); + FocusListener.setFocusListener(focusListener); + } + + @AfterEach + void verifyNoTypeThrashing() { + FocusListener.setFocusListener(null); + Assertions.assertTrue(focusListener.checkThresholds(THRESHOLD), "Threshold exceeded, check logs."); + } + + private void doTest(Executable r) throws Throwable { + for (int i = 0; i < THRESHOLD * 2; i++) { + r.execute(); + } + } + + @Test + @Disabled + public void sample() throws Throwable { + // example test. If you enable this, it will fail, and you can see what a type pollution error looks like. + + interface A { + } + + interface B { + } + + class Concrete implements A, B { + } + + Object c = new Concrete(); + AtomicInteger j = new AtomicInteger(); + doTest(() -> { + if (c instanceof A) { + j.incrementAndGet(); + } + if (c instanceof B) { + j.incrementAndGet(); + } + }); + System.out.println(j); + } + + @Test + public void deserializeRecordWithList() throws Throwable { + ObjectMapper mapper = JsonMapper.builder().build(); + ListRecord input = new ListRecord(List.of("foo")); + String json = mapper.writeValueAsString(input); + doTest(() -> Assertions.assertEquals(input, mapper.readValue(json, ListRecord.class))); + } + + record ListRecord(List list) { + } + + @Test + public void deserializeRecordWithSet() throws Throwable { + ObjectMapper mapper = JsonMapper.builder().build(); + SetRecord input = new SetRecord(Set.of("foo")); + String json = mapper.writeValueAsString(input); + doTest(() -> Assertions.assertEquals(input, mapper.readValue(json, SetRecord.class))); + } + + record SetRecord(Set list) { + } +} diff --git a/src/test/java/com/fasterxml/jackson/databind/PrimarySuite.java b/src/test/java/com/fasterxml/jackson/databind/PrimarySuite.java new file mode 100644 index 0000000000..32db66b373 --- /dev/null +++ b/src/test/java/com/fasterxml/jackson/databind/PrimarySuite.java @@ -0,0 +1,16 @@ +package com.fasterxml.jackson.databind; + +import org.junit.platform.suite.api.ExcludeClassNamePatterns; +import org.junit.platform.suite.api.ExcludePackages; +import org.junit.platform.suite.api.SelectPackages; +import org.junit.platform.suite.api.Suite; + +@Suite +@SelectPackages("com.fasterxml.jackson.databind") +@ExcludePackages("com.fasterxml.jackson.databind.typepollution") +@ExcludeClassNamePatterns({ + "com\\.fasterxml\\.jackson\\.databind\\.MapperFootprintTest", + ".*\\.failing\\..*" +}) +public class PrimarySuite { +} From 6c74d0ee74a4d2dd30a65a7076527cfb7b4a44f4 Mon Sep 17 00:00:00 2001 From: yawkat Date: Tue, 10 Dec 2024 14:48:47 +0100 Subject: [PATCH 2/8] fix imports --- .../std/StringCollectionDeserializer.java | 20 ++++++++----------- .../typepollution/TypePollutionTest.java | 18 +++++++---------- 2 files changed, 15 insertions(+), 23 deletions(-) diff --git a/src/main/java/com/fasterxml/jackson/databind/deser/std/StringCollectionDeserializer.java b/src/main/java/com/fasterxml/jackson/databind/deser/std/StringCollectionDeserializer.java index 78d844302e..5b567a4776 100644 --- a/src/main/java/com/fasterxml/jackson/databind/deser/std/StringCollectionDeserializer.java +++ b/src/main/java/com/fasterxml/jackson/databind/deser/std/StringCollectionDeserializer.java @@ -1,14 +1,14 @@ package com.fasterxml.jackson.databind.deser.std; -import com.fasterxml.jackson.annotation.JsonFormat; +import java.io.IOException; +import java.util.ArrayList; +import java.util.Collection; +import java.util.HashSet; +import java.util.Objects; + import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.core.JsonToken; -import com.fasterxml.jackson.databind.BeanProperty; -import com.fasterxml.jackson.databind.DeserializationContext; -import com.fasterxml.jackson.databind.DeserializationFeature; -import com.fasterxml.jackson.databind.JavaType; -import com.fasterxml.jackson.databind.JsonDeserializer; -import com.fasterxml.jackson.databind.JsonMappingException; +import com.fasterxml.jackson.databind.*; import com.fasterxml.jackson.databind.annotation.JacksonStdImpl; import com.fasterxml.jackson.databind.cfg.CoercionAction; import com.fasterxml.jackson.databind.cfg.CoercionInputShape; @@ -19,11 +19,7 @@ import com.fasterxml.jackson.databind.jsontype.TypeDeserializer; import com.fasterxml.jackson.databind.type.LogicalType; -import java.io.IOException; -import java.util.ArrayList; -import java.util.Collection; -import java.util.HashSet; -import java.util.Objects; +import com.fasterxml.jackson.annotation.JsonFormat; /** * Specifically optimized version for {@link java.util.Collection}s diff --git a/src/test-jdk17/java/com/fasterxml/jackson/databind/typepollution/TypePollutionTest.java b/src/test-jdk17/java/com/fasterxml/jackson/databind/typepollution/TypePollutionTest.java index 4e1a2b5d70..cf0972470e 100644 --- a/src/test-jdk17/java/com/fasterxml/jackson/databind/typepollution/TypePollutionTest.java +++ b/src/test-jdk17/java/com/fasterxml/jackson/databind/typepollution/TypePollutionTest.java @@ -1,21 +1,17 @@ package com.fasterxml.jackson.databind.typepollution; -import com.fasterxml.jackson.databind.ObjectMapper; -import com.fasterxml.jackson.databind.json.JsonMapper; +import java.util.List; +import java.util.Set; +import java.util.concurrent.atomic.AtomicInteger; + import io.micronaut.test.typepollution.FocusListener; import io.micronaut.test.typepollution.ThresholdFocusListener; import io.micronaut.test.typepollution.TypePollutionTransformer; -import org.junit.jupiter.api.AfterEach; -import org.junit.jupiter.api.Assertions; -import org.junit.jupiter.api.BeforeAll; -import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Disabled; -import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.*; import org.junit.jupiter.api.function.Executable; -import java.util.List; -import java.util.Set; -import java.util.concurrent.atomic.AtomicInteger; +import com.fasterxml.jackson.databind.ObjectMapper; +import com.fasterxml.jackson.databind.json.JsonMapper; public class TypePollutionTest { private static final int THRESHOLD = 1000; From 4b4ba00df9146500cccec60c3fbc09b0b5fe8a47 Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Fri, 20 Dec 2024 18:46:51 -0800 Subject: [PATCH 3/8] Minor cosmetic changes --- .../deser/std/StringCollectionDeserializer.java | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/src/main/java/com/fasterxml/jackson/databind/deser/std/StringCollectionDeserializer.java b/src/main/java/com/fasterxml/jackson/databind/deser/std/StringCollectionDeserializer.java index 5b567a4776..8d22556216 100644 --- a/src/main/java/com/fasterxml/jackson/databind/deser/std/StringCollectionDeserializer.java +++ b/src/main/java/com/fasterxml/jackson/databind/deser/std/StringCollectionDeserializer.java @@ -1,13 +1,13 @@ package com.fasterxml.jackson.databind.deser.std; import java.io.IOException; -import java.util.ArrayList; -import java.util.Collection; -import java.util.HashSet; -import java.util.Objects; +import java.util.*; + +import com.fasterxml.jackson.annotation.JsonFormat; import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.core.JsonToken; + import com.fasterxml.jackson.databind.*; import com.fasterxml.jackson.databind.annotation.JacksonStdImpl; import com.fasterxml.jackson.databind.cfg.CoercionAction; @@ -19,8 +19,6 @@ import com.fasterxml.jackson.databind.jsontype.TypeDeserializer; import com.fasterxml.jackson.databind.type.LogicalType; -import com.fasterxml.jackson.annotation.JsonFormat; - /** * Specifically optimized version for {@link java.util.Collection}s * that contain String values; reason is that this is a very common @@ -331,6 +329,7 @@ private final Collection handleNonArray(JsonParser p, DeserializationCon return result; } + // @since 2.18 @SuppressWarnings("unchecked") private static Collection castToCollection(Object o) { if (o != null) { From e9a3043265e2e93d234b0093ad1e2532e6602efe Mon Sep 17 00:00:00 2001 From: Tatu Saloranta Date: Fri, 20 Dec 2024 19:43:29 -0800 Subject: [PATCH 4/8] Minor fixes to pom.xml --- pom.xml | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/pom.xml b/pom.xml index 2e93f3cfb2..8755bb764b 100644 --- a/pom.xml +++ b/pom.xml @@ -211,8 +211,7 @@ javax.measure:jsr-275 - com.fasterxml.jackson.databind.MapperFootprintTest - **/failing/**/*.java + com.fasterxml.jackson.databind.MapperFootprintTest org.assertj assertj-core @@ -146,12 +140,6 @@ 0.16 test - - io.micronaut.test - micronaut-test-type-pollution - 4.6.2 - test - @@ -166,6 +154,22 @@ ${version.mockito} test + + + + org.junit.platform + junit-platform-suite-engine + 1.10.2 + test + + + io.micronaut.test + micronaut-test-type-pollution + 4.6.2 + test + @@ -467,21 +467,8 @@ org.apache.maven.plugins maven-surefire-plugin - --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED -XX:+EnableDynamicAgentLoading + --add-opens=java.base/java.lang=ALL-UNNAMED --add-opens=java.base/java.util=ALL-UNNAMED - - - type-pollution-test - test - - test - - - com.fasterxml.jackson.databind.typepollution.TypePollutionSuite - 1 - - -