From c960d2308607eb11107dea18e168997938a3692c Mon Sep 17 00:00:00 2001 From: Ikhun Um Date: Mon, 18 Mar 2024 21:46:15 +0900 Subject: [PATCH] Fix to thread-safely load Thrift classes for old libthrift (< 0.9.3) (#5497) Motivation: In old Thrift versions (< 0.9.3), the multi-threaded environment was not considered during the initialization process for the Thrift class. A workaround was committed at #4688 but only applied to DocService. This problem can occur not only in `DocService` but also when creating Thrift clients and other parts, so it would be desirable to use `ThriftMetadataAccess.getStructMetaDataMap()` for places where `FieldMetaData.getStructMetaDataMap()` is used. ```java // When creating Thrift clients java.lang.IllegalArgumentException: failed to retrieve function metadata: ... at com.linecorp.armeria.internal.common.thrift.ThriftServiceMetadata.registerFunction(ThriftServiceMetadata.java:239) at com.linecorp.armeria.internal.common.thrift.ThriftServiceMetadata.lambda$init$2(ThriftServiceMetadata.java:117) at java.base/java.util.HashMap.forEach(HashMap.java:1337) at java.base/java.util.Collections$UnmodifiableMap.forEach(Collections.java:1505) at com.linecorp.armeria.internal.common.thrift.ThriftServiceMetadata.init(ThriftServiceMetadata.java:116) at com.linecorp.armeria.internal.common.thrift.ThriftServiceMetadata.(ThriftServiceMetadata.java:85) at java.base/java.util.concurrent.ConcurrentHashMap.computeIfAbsent(ConcurrentHashMap.java:1705) at com.linecorp.armeria.internal.client.thrift.THttpClientDelegate.metadata(THttpClientDelegate.java:216) at com.linecorp.armeria.internal.client.thrift.THttpClientDelegate.execute(THttpClientDelegate.java:122) at com.linecorp.armeria.internal.client.thrift.THttpClientDelegate.execute(THttpClientDelegate.java:78) Caused by: java.lang.NullPointerException: null at com.linecorp.armeria.internal.common.thrift.ThriftFunction.(ThriftFunction.java:104) at com.linecorp.armeria.internal.common.thrift.ThriftFunction.(ThriftFunction.java:66) at com.linecorp.armeria.internal.common.thrift.ThriftServiceMetadata.registerFunction(ThriftServiceMetadata.java:229) ... 33 common frames omitted // When creating DocService Caused by: java.lang.NullPointerException: null at com.linecorp.armeria.internal.server.thrift.ThriftDescriptiveTypeInfoProvider.newStructInfo(ThriftDescriptiveTypeInfoProvider.java:322) at com.linecorp.armeria.internal.server.thrift.ThriftDescriptiveTypeInfoProvider.newDescriptiveTypeInfo(ThriftDescriptiveTypeInfoProvider.java:101) at com.linecorp.armeria.server.docs.DocService$SpecificationLoader.lambda$composeDescriptiveTypeInfoProvider$9(DocService.java:386) ``` Reference: - https://issues.apache.org/jira/browse/THRIFT-1618 - https://github.com/apache/thrift/commit/4a78c6eb8670cbb664a199b1c98518033e51e525 Modifications: - Replace `FieldMetaData.getStructMetaDataMap()` with `ThriftMetadataAccess.getStructMetaDataMap()` to thread-safely initialize Thrift classes. Result: You no longer see `NullPointerException` when creating Thrift clients in a multi-threaded environment. --- .../armeria/common/thrift/text/StructContext.java | 3 ++- .../armeria/internal/common/thrift/ThriftFunction.java | 2 +- .../thrift/ThriftMetadataAccess.java | 10 +++++----- .../thrift/ThriftDescriptiveTypeInfoProvider.java | 1 + .../internal/server/thrift/ThriftDocServicePlugin.java | 1 + .../linecorp/armeria/server/thrift/THttpService.java | 4 ++-- .../thrift/ThriftMetadataAccessTest.java | 2 +- 7 files changed, 13 insertions(+), 10 deletions(-) rename thrift/thrift0.13/src/main/java/com/linecorp/armeria/internal/{server => common}/thrift/ThriftMetadataAccess.java (90%) rename thrift/thrift0.13/src/test/java/com/linecorp/armeria/internal/{server => common}/thrift/ThriftMetadataAccessTest.java (95%) diff --git a/thrift/thrift0.13/src/main/java/com/linecorp/armeria/common/thrift/text/StructContext.java b/thrift/thrift0.13/src/main/java/com/linecorp/armeria/common/thrift/text/StructContext.java index fcaf6978769..55a93c90d76 100644 --- a/thrift/thrift0.13/src/main/java/com/linecorp/armeria/common/thrift/text/StructContext.java +++ b/thrift/thrift0.13/src/main/java/com/linecorp/armeria/common/thrift/text/StructContext.java @@ -58,6 +58,7 @@ import com.linecorp.armeria.common.annotation.Nullable; import com.linecorp.armeria.common.util.SystemInfo; +import com.linecorp.armeria.internal.common.thrift.ThriftMetadataAccess; /** * A struct parsing context. Builds a map from field name to TField. @@ -170,7 +171,7 @@ private , F extends TFieldIdEnum> Map comp // Get the metaDataMap for this Thrift class @SuppressWarnings("unchecked") final Map metaDataMap = - FieldMetaData.getStructMetaDataMap((Class) clazz); + ThriftMetadataAccess.getStructMetaDataMap((Class) clazz); for (Entry e : metaDataMap.entrySet()) { final String fieldName = e.getKey().getFieldName(); diff --git a/thrift/thrift0.13/src/main/java/com/linecorp/armeria/internal/common/thrift/ThriftFunction.java b/thrift/thrift0.13/src/main/java/com/linecorp/armeria/internal/common/thrift/ThriftFunction.java index 5e425bf8fcf..4898eaa6598 100644 --- a/thrift/thrift0.13/src/main/java/com/linecorp/armeria/internal/common/thrift/ThriftFunction.java +++ b/thrift/thrift0.13/src/main/java/com/linecorp/armeria/internal/common/thrift/ThriftFunction.java @@ -106,7 +106,7 @@ private , F extends TFieldIdEnum> ThriftFunction( //noinspection RedundantCast @SuppressWarnings("unchecked") final Map metaDataMap = - (Map) FieldMetaData.getStructMetaDataMap( + (Map) ThriftMetadataAccess.getStructMetaDataMap( (Class) resultType); for (Entry e : metaDataMap.entrySet()) { diff --git a/thrift/thrift0.13/src/main/java/com/linecorp/armeria/internal/server/thrift/ThriftMetadataAccess.java b/thrift/thrift0.13/src/main/java/com/linecorp/armeria/internal/common/thrift/ThriftMetadataAccess.java similarity index 90% rename from thrift/thrift0.13/src/main/java/com/linecorp/armeria/internal/server/thrift/ThriftMetadataAccess.java rename to thrift/thrift0.13/src/main/java/com/linecorp/armeria/internal/common/thrift/ThriftMetadataAccess.java index cf94e588be6..7e61bad7d06 100644 --- a/thrift/thrift0.13/src/main/java/com/linecorp/armeria/internal/server/thrift/ThriftMetadataAccess.java +++ b/thrift/thrift0.13/src/main/java/com/linecorp/armeria/internal/common/thrift/ThriftMetadataAccess.java @@ -1,5 +1,5 @@ /* - * Copyright 2023 LINE Corporation + * Copyright 2024 LINE Corporation * * LINE Corporation licenses this file to you under the Apache License, * version 2.0 (the "License"); you may not use this file except in compliance @@ -14,7 +14,7 @@ * under the License. */ -package com.linecorp.armeria.internal.server.thrift; +package com.linecorp.armeria.internal.common.thrift; import java.io.BufferedReader; import java.io.InputStreamReader; @@ -30,7 +30,7 @@ import com.google.common.annotations.VisibleForTesting; -final class ThriftMetadataAccess { +public final class ThriftMetadataAccess { private static final Logger logger = LoggerFactory.getLogger(ThriftMetadataAccess.class); @@ -62,8 +62,8 @@ static boolean needsPreInitialization(Properties properties) { } @SuppressWarnings("unchecked") - public static , F extends TFieldIdEnum> - Map getStructMetaDataMap(Class clazz) { + public static synchronized , F extends TFieldIdEnum> + Map getStructMetaDataMap(Class clazz) { // Pre-initialize classes if there is a jar in the classpath with armeria-thrift <= 0.14 // See the following issue for the motivation of pre-initializing classes // https://issues.apache.org/jira/browse/THRIFT-5430 diff --git a/thrift/thrift0.13/src/main/java/com/linecorp/armeria/internal/server/thrift/ThriftDescriptiveTypeInfoProvider.java b/thrift/thrift0.13/src/main/java/com/linecorp/armeria/internal/server/thrift/ThriftDescriptiveTypeInfoProvider.java index f49bcaa0510..a76e39989b6 100644 --- a/thrift/thrift0.13/src/main/java/com/linecorp/armeria/internal/server/thrift/ThriftDescriptiveTypeInfoProvider.java +++ b/thrift/thrift0.13/src/main/java/com/linecorp/armeria/internal/server/thrift/ThriftDescriptiveTypeInfoProvider.java @@ -48,6 +48,7 @@ import com.google.common.annotations.VisibleForTesting; import com.linecorp.armeria.common.annotation.Nullable; +import com.linecorp.armeria.internal.common.thrift.ThriftMetadataAccess; import com.linecorp.armeria.server.docs.DescriptiveTypeInfo; import com.linecorp.armeria.server.docs.DescriptiveTypeInfoProvider; import com.linecorp.armeria.server.docs.EnumInfo; diff --git a/thrift/thrift0.13/src/main/java/com/linecorp/armeria/internal/server/thrift/ThriftDocServicePlugin.java b/thrift/thrift0.13/src/main/java/com/linecorp/armeria/internal/server/thrift/ThriftDocServicePlugin.java index 5f0f3d97de2..4679254d179 100644 --- a/thrift/thrift0.13/src/main/java/com/linecorp/armeria/internal/server/thrift/ThriftDocServicePlugin.java +++ b/thrift/thrift0.13/src/main/java/com/linecorp/armeria/internal/server/thrift/ThriftDocServicePlugin.java @@ -50,6 +50,7 @@ import com.linecorp.armeria.common.HttpMethod; import com.linecorp.armeria.common.annotation.Nullable; import com.linecorp.armeria.common.thrift.ThriftProtocolFactories; +import com.linecorp.armeria.internal.common.thrift.ThriftMetadataAccess; import com.linecorp.armeria.server.Route; import com.linecorp.armeria.server.RoutePathType; import com.linecorp.armeria.server.Service; diff --git a/thrift/thrift0.13/src/main/java/com/linecorp/armeria/server/thrift/THttpService.java b/thrift/thrift0.13/src/main/java/com/linecorp/armeria/server/thrift/THttpService.java index 64057be68b4..a20482b4f33 100644 --- a/thrift/thrift0.13/src/main/java/com/linecorp/armeria/server/thrift/THttpService.java +++ b/thrift/thrift0.13/src/main/java/com/linecorp/armeria/server/thrift/THttpService.java @@ -34,7 +34,6 @@ import org.apache.thrift.TBase; import org.apache.thrift.TException; import org.apache.thrift.TFieldIdEnum; -import org.apache.thrift.meta_data.FieldMetaData; import org.apache.thrift.protocol.TMessage; import org.apache.thrift.protocol.TMessageType; import org.apache.thrift.protocol.TProtocol; @@ -74,6 +73,7 @@ import com.linecorp.armeria.internal.common.thrift.TByteBufTransport; import com.linecorp.armeria.internal.common.thrift.ThriftFieldAccess; import com.linecorp.armeria.internal.common.thrift.ThriftFunction; +import com.linecorp.armeria.internal.common.thrift.ThriftMetadataAccess; import com.linecorp.armeria.internal.common.thrift.ThriftProtocolUtil; import com.linecorp.armeria.internal.server.annotation.DecoratorAnnotationUtil.DecoratorAndOrder; import com.linecorp.armeria.server.DecoratingService; @@ -653,7 +653,7 @@ private static RpcRequest toRpcRequest(Class serviceType, String method, TBas // NB: The map returned by FieldMetaData.getStructMetaDataMap() is an EnumMap, // so the parameter ordering is preserved correctly during iteration. final Set fields = - FieldMetaData.getStructMetaDataMap(thriftArgs.getClass()).keySet(); + ThriftMetadataAccess.getStructMetaDataMap(thriftArgs.getClass()).keySet(); // Handle the case where the number of arguments is 0 or 1. final int numFields = fields.size(); diff --git a/thrift/thrift0.13/src/test/java/com/linecorp/armeria/internal/server/thrift/ThriftMetadataAccessTest.java b/thrift/thrift0.13/src/test/java/com/linecorp/armeria/internal/common/thrift/ThriftMetadataAccessTest.java similarity index 95% rename from thrift/thrift0.13/src/test/java/com/linecorp/armeria/internal/server/thrift/ThriftMetadataAccessTest.java rename to thrift/thrift0.13/src/test/java/com/linecorp/armeria/internal/common/thrift/ThriftMetadataAccessTest.java index 48b217f3881..9da546d311c 100644 --- a/thrift/thrift0.13/src/test/java/com/linecorp/armeria/internal/server/thrift/ThriftMetadataAccessTest.java +++ b/thrift/thrift0.13/src/test/java/com/linecorp/armeria/internal/common/thrift/ThriftMetadataAccessTest.java @@ -14,7 +14,7 @@ * under the License. */ -package com.linecorp.armeria.internal.server.thrift; +package com.linecorp.armeria.internal.common.thrift; import static org.assertj.core.api.Assertions.assertThat;