Skip to content

Commit

Permalink
Fix to thread-safely load Thrift classes for libthrift < 0.9.3
Browse files Browse the repository at this point in the history
Motivation:

In old Thrift versions (< 0.9.3), the multi-thread environment was not
considered during the initialization process for the Thrift class.
A workarournd for that was committed at line#4688, but it 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)
```

Referece:
- https://issues.apache.org/jira/browse/THRIFT-1618
- apache/thrift@4a78c6e

Motifications:

- 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.
  • Loading branch information
ikhoon committed Mar 13, 2024
1 parent 4bfa172 commit da8ce85
Show file tree
Hide file tree
Showing 7 changed files with 13 additions and 10 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down Expand Up @@ -170,7 +171,7 @@ private <T extends TBase<T, F>, F extends TFieldIdEnum> Map<String, TField> comp
// Get the metaDataMap for this Thrift class
@SuppressWarnings("unchecked")
final Map<? extends TFieldIdEnum, FieldMetaData> metaDataMap =
FieldMetaData.getStructMetaDataMap((Class<T>) clazz);
ThriftMetadataAccess.getStructMetaDataMap((Class<T>) clazz);

for (Entry<? extends TFieldIdEnum, FieldMetaData> e : metaDataMap.entrySet()) {
final String fieldName = e.getKey().getFieldName();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -106,7 +106,7 @@ private <T extends TBase<T, F>, F extends TFieldIdEnum> ThriftFunction(
//noinspection RedundantCast
@SuppressWarnings("unchecked")
final Map<TFieldIdEnum, FieldMetaData> metaDataMap =
(Map<TFieldIdEnum, FieldMetaData>) FieldMetaData.getStructMetaDataMap(
(Map<TFieldIdEnum, FieldMetaData>) ThriftMetadataAccess.getStructMetaDataMap(
(Class<T>) resultType);

for (Entry<TFieldIdEnum, FieldMetaData> e : metaDataMap.entrySet()) {
Expand Down
Original file line number Diff line number Diff line change
@@ -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
Expand All @@ -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;
Expand All @@ -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);

Expand Down Expand Up @@ -62,8 +62,8 @@ static boolean needsPreInitialization(Properties properties) {
}

@SuppressWarnings("unchecked")
public static <T extends TBase<T, F>, F extends TFieldIdEnum>
Map<?, FieldMetaData> getStructMetaDataMap(Class<?> clazz) {
public static synchronized <T extends TBase<T, F>, F extends TFieldIdEnum>
Map<? extends TFieldIdEnum, FieldMetaData> 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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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<? extends TFieldIdEnum> 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();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand Down

0 comments on commit da8ce85

Please sign in to comment.