Skip to content

Commit c960d23

Browse files
authored
Fix to thread-safely load Thrift classes for old libthrift (< 0.9.3) (line#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 line#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.<init>(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.<init>(ThriftFunction.java:104) at com.linecorp.armeria.internal.common.thrift.ThriftFunction.<init>(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 - apache/thrift@4a78c6e 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.
1 parent ccb29fe commit c960d23

File tree

7 files changed

+13
-10
lines changed

7 files changed

+13
-10
lines changed

thrift/thrift0.13/src/main/java/com/linecorp/armeria/common/thrift/text/StructContext.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,7 @@
5858

5959
import com.linecorp.armeria.common.annotation.Nullable;
6060
import com.linecorp.armeria.common.util.SystemInfo;
61+
import com.linecorp.armeria.internal.common.thrift.ThriftMetadataAccess;
6162

6263
/**
6364
* A struct parsing context. Builds a map from field name to TField.
@@ -170,7 +171,7 @@ private <T extends TBase<T, F>, F extends TFieldIdEnum> Map<String, TField> comp
170171
// Get the metaDataMap for this Thrift class
171172
@SuppressWarnings("unchecked")
172173
final Map<? extends TFieldIdEnum, FieldMetaData> metaDataMap =
173-
FieldMetaData.getStructMetaDataMap((Class<T>) clazz);
174+
ThriftMetadataAccess.getStructMetaDataMap((Class<T>) clazz);
174175

175176
for (Entry<? extends TFieldIdEnum, FieldMetaData> e : metaDataMap.entrySet()) {
176177
final String fieldName = e.getKey().getFieldName();

thrift/thrift0.13/src/main/java/com/linecorp/armeria/internal/common/thrift/ThriftFunction.java

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ private <T extends TBase<T, F>, F extends TFieldIdEnum> ThriftFunction(
106106
//noinspection RedundantCast
107107
@SuppressWarnings("unchecked")
108108
final Map<TFieldIdEnum, FieldMetaData> metaDataMap =
109-
(Map<TFieldIdEnum, FieldMetaData>) FieldMetaData.getStructMetaDataMap(
109+
(Map<TFieldIdEnum, FieldMetaData>) ThriftMetadataAccess.getStructMetaDataMap(
110110
(Class<T>) resultType);
111111

112112
for (Entry<TFieldIdEnum, FieldMetaData> e : metaDataMap.entrySet()) {
Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
/*
2-
* Copyright 2023 LINE Corporation
2+
* Copyright 2024 LINE Corporation
33
*
44
* LINE Corporation licenses this file to you under the Apache License,
55
* version 2.0 (the "License"); you may not use this file except in compliance
@@ -14,7 +14,7 @@
1414
* under the License.
1515
*/
1616

17-
package com.linecorp.armeria.internal.server.thrift;
17+
package com.linecorp.armeria.internal.common.thrift;
1818

1919
import java.io.BufferedReader;
2020
import java.io.InputStreamReader;
@@ -30,7 +30,7 @@
3030

3131
import com.google.common.annotations.VisibleForTesting;
3232

33-
final class ThriftMetadataAccess {
33+
public final class ThriftMetadataAccess {
3434

3535
private static final Logger logger = LoggerFactory.getLogger(ThriftMetadataAccess.class);
3636

@@ -62,8 +62,8 @@ static boolean needsPreInitialization(Properties properties) {
6262
}
6363

6464
@SuppressWarnings("unchecked")
65-
public static <T extends TBase<T, F>, F extends TFieldIdEnum>
66-
Map<?, FieldMetaData> getStructMetaDataMap(Class<?> clazz) {
65+
public static synchronized <T extends TBase<T, F>, F extends TFieldIdEnum>
66+
Map<? extends TFieldIdEnum, FieldMetaData> getStructMetaDataMap(Class<?> clazz) {
6767
// Pre-initialize classes if there is a jar in the classpath with armeria-thrift <= 0.14
6868
// See the following issue for the motivation of pre-initializing classes
6969
// https://issues.apache.org/jira/browse/THRIFT-5430

thrift/thrift0.13/src/main/java/com/linecorp/armeria/internal/server/thrift/ThriftDescriptiveTypeInfoProvider.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,7 @@
4848
import com.google.common.annotations.VisibleForTesting;
4949

5050
import com.linecorp.armeria.common.annotation.Nullable;
51+
import com.linecorp.armeria.internal.common.thrift.ThriftMetadataAccess;
5152
import com.linecorp.armeria.server.docs.DescriptiveTypeInfo;
5253
import com.linecorp.armeria.server.docs.DescriptiveTypeInfoProvider;
5354
import com.linecorp.armeria.server.docs.EnumInfo;

thrift/thrift0.13/src/main/java/com/linecorp/armeria/internal/server/thrift/ThriftDocServicePlugin.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@
5050
import com.linecorp.armeria.common.HttpMethod;
5151
import com.linecorp.armeria.common.annotation.Nullable;
5252
import com.linecorp.armeria.common.thrift.ThriftProtocolFactories;
53+
import com.linecorp.armeria.internal.common.thrift.ThriftMetadataAccess;
5354
import com.linecorp.armeria.server.Route;
5455
import com.linecorp.armeria.server.RoutePathType;
5556
import com.linecorp.armeria.server.Service;

thrift/thrift0.13/src/main/java/com/linecorp/armeria/server/thrift/THttpService.java

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,6 @@
3434
import org.apache.thrift.TBase;
3535
import org.apache.thrift.TException;
3636
import org.apache.thrift.TFieldIdEnum;
37-
import org.apache.thrift.meta_data.FieldMetaData;
3837
import org.apache.thrift.protocol.TMessage;
3938
import org.apache.thrift.protocol.TMessageType;
4039
import org.apache.thrift.protocol.TProtocol;
@@ -74,6 +73,7 @@
7473
import com.linecorp.armeria.internal.common.thrift.TByteBufTransport;
7574
import com.linecorp.armeria.internal.common.thrift.ThriftFieldAccess;
7675
import com.linecorp.armeria.internal.common.thrift.ThriftFunction;
76+
import com.linecorp.armeria.internal.common.thrift.ThriftMetadataAccess;
7777
import com.linecorp.armeria.internal.common.thrift.ThriftProtocolUtil;
7878
import com.linecorp.armeria.internal.server.annotation.DecoratorAnnotationUtil.DecoratorAndOrder;
7979
import com.linecorp.armeria.server.DecoratingService;
@@ -653,7 +653,7 @@ private static RpcRequest toRpcRequest(Class<?> serviceType, String method, TBas
653653
// NB: The map returned by FieldMetaData.getStructMetaDataMap() is an EnumMap,
654654
// so the parameter ordering is preserved correctly during iteration.
655655
final Set<? extends TFieldIdEnum> fields =
656-
FieldMetaData.getStructMetaDataMap(thriftArgs.getClass()).keySet();
656+
ThriftMetadataAccess.getStructMetaDataMap(thriftArgs.getClass()).keySet();
657657

658658
// Handle the case where the number of arguments is 0 or 1.
659659
final int numFields = fields.size();
Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -14,7 +14,7 @@
1414
* under the License.
1515
*/
1616

17-
package com.linecorp.armeria.internal.server.thrift;
17+
package com.linecorp.armeria.internal.common.thrift;
1818

1919
import static org.assertj.core.api.Assertions.assertThat;
2020

0 commit comments

Comments
 (0)