Fix an intermittent deadlock when `DocService` is loaded for a Thrift service #4688

jrhee17 · 2023-02-21T22:16:02Z

Motivation:

We use thrift/grpc internal APIs when constructing DocService.
Following #4491, these internal APIs are called concurrently with service logic.
Due to a thrift bug, a deadlock can occur when FieldMetaData.getStructMetaDataMap is called for a thrift class while the class is initialized.
https://issues.apache.org/jira/browse/THRIFT-5430
This PR attempts to circumvent this issue by pre-loading the class for affected versions (<= armeria-thrift 0.14)

I've also double checked that other *MetaData implementations don't have a similar implementation.

Modifications:

Add a ThriftMetadataAccess utility class
When ThriftMetadataAccess is instantiated, check if (<= armeria-thrift 0.14) is in the classpath
If the vulnerable artifact is loaded, then pre-instantiate thrift classes before FieldMetaData.getStructMetaDataMap is called from DocService

Result:

A deadlock is no longer observed when loading DocService for thrift services where <= armeria-thrift 0.14

ikhoon

Looks good. Left only minor nits.

ikhoon · 2023-02-28T08:19:50Z

...rift0.13/src/main/java/com/linecorp/armeria/internal/common/thrift/ThriftMetadataAccess.java

+ }
+ ThriftMetadataAccess.preInitializeThriftClass = preInitializeThriftClass;
+ } catch (Exception e) {
+ logger.trace("Unexpected exception while determining the 'armeria-thrift' version: ", e);


com.linecorp.armeria.versions.properties can be removed while creating a uber jar or shading. A debug level may be useful for that users.

Suggested change

logger.trace("Unexpected exception while determining the 'armeria-thrift' version: ", e);

logger.debug("Unexpected exception while determining the 'armeria-thrift' version: ", e);

Should we also leave a debug message if versionPropertiesUrls is empty?

ikhoon · 2023-02-28T08:35:20Z

...rift0.13/src/main/java/com/linecorp/armeria/internal/common/thrift/ThriftMetadataAccess.java

+ }
+
+ @VisibleForTesting
+ static boolean needPreInitialize(Properties props) {


Thrid-person verb is commonly used for boolean methods.
https://softwareengineering.stackexchange.com/questions/404650/what-form-of-verb-to-use-imperative-verb-or-third-person-verb-in-programming

Suggested change

static boolean needPreInitialize(Properties props) {

static boolean needsPreInitialization(Properties props) {

ikhoon · 2023-02-28T08:42:42Z

...rift0.13/src/main/java/com/linecorp/armeria/internal/common/thrift/ThriftMetadataAccess.java

+
+import com.google.common.annotations.VisibleForTesting;
+
+public final class ThriftMetadataAccess {


It would be better to keep a non-public scope if possible. How about moving ThriftMetadataAccess to com.linecorp.armeria.internal.server.thrift and making it package-private? We may move ThriftMetadataAccess to the internal package when clients need to use it.

Suggested change

public final class ThriftMetadataAccess {

final class ThriftMetadataAccess {

ikhoon · 2023-02-28T08:47:13Z

...ft0.13/src/test/java/com/linecorp/armeria/internal/server/thrift/ThriftClassLoadingTest.java

+ }
+
+ @Override
+ public Class<?> loadClass(String name) throws ClassNotFoundException {


e1 and e2 may access this method simultaneously.

Suggested change

public Class<?> loadClass(String name) throws ClassNotFoundException {

public synchronized Class<?> loadClass(String name) throws ClassNotFoundException {

trustin · 2023-03-07T14:34:07Z

...rift0.13/src/main/java/com/linecorp/armeria/internal/server/thrift/ThriftMetadataAccess.java

+ final Enumeration<URL> versionPropertiesUrls =
+ ThriftMetadataAccess.class.getClassLoader().getResources(
+ "META-INF/com.linecorp.armeria.versions.properties");


Can't we just do Version.getAll(...).keySet() to get the list of the artifact IDs?

Can't we just always pre-initialize the classes?

What do you think about adding an empty resource file like src/main/resources/com/linecorp/armeria/internal/common/thrift/requires_struct_preinit to all thrift modules older than 0.15? We could enable the workaround only if that resource file exists.

Given the cost of failure to look for a resource file might be a little bit higher, we could always add a properties file:

If pre-init is required:

structPreinitRequired=true

If pre-init is not required:

<empty file>

Can't we just do Version.getAll(...).keySet() to get the list of the artifact IDs?

Didn't know this class existed 😅 It's probably better to use this.

Can't we just always pre-initialize the classes?

I think I prefer making an explicit branch statement for these type of changes:

If we decide to deprecate thrift <= 0.14 modules, we can remove the related logic safely

The reason for pre-initialization is embedded in the code

I do agree that the logic doesn't really affect performance and am open to just preinitializing always if others feel this is simpler.

What do you think about adding an empty resource file
we could always add a properties file

~~I don't even think this has to be a resource, but it can also be a class file that has a different static value for each thrift module.~~ <- On second thought, this doesn't work if multiple armeria-thrift artifacts are added.
I didn't want to further complicate the thrift file copying logic, but I think this is also an option.

I think I prefer just using Versions.all() like you mentioned, but let me know if you feel differently.

this doesn't work if multiple armeria-thrift artifacts are added.

Doesn't a user have a bigger problem if their classpath has multiple armeria-thrift artifacts? We could leave a warning and fall back to safe mode (always preinit)?

~~I don't even think this has to be a resource, but it can also be a class file that has a different static value for each thrift module.~~ <- On second thought, this doesn't work if multiple armeria-thrift artifacts are added.

We could leave a warning and fall back to safe mode (always preinit)?

I understood what you meant to be leave a warning if multiple classes exist, am I understanding correctly?
I didn't think it was possible to detect if there are multiple classes with the same package/name/module.

This won't be a problem if we define a separate resource instead of class, but I don't see much benefit in using a dedicated resource over just Versions.all().

(always preinit)?

or did you mean to just always preinit? 😅

I didn't think it was possible to detect if there are multiple classes with the same package/name/module.

You can actually:

Enumeration<URL> urls = getClass().getClassLoader().getResources("com/foo/bar.txt"); // or .class while (urls.hasMoreElements()) { URL url = urls.nextElement(); InputStream inputStream = url.openStream(); // do something with the input stream }

Otherwise, how could Versions.all() fetch the com.linecorp.armeria.versions.properties files from all artifacts?

By the way, I meant putting some .properties file rather than looking for .class files. I think having a separate resource is still better than extracting version number from artifact ID because it doesn't require any string matching, which could be broken at some point.

I think having a separate resource is still better than extracting version number from artifact ID because it doesn't require any string matching, which could be broken at some point.

I see 😄 Understood.

...rift0.13/src/main/java/com/linecorp/armeria/internal/server/thrift/ThriftMetadataAccess.java

...thrift0.13/src/main/resources/com/linecorp/armeria/internal/common/thrift-options.properties

…-deadlock

ikhoon · 2023-03-23T15:51:50Z

...rift0.13/src/main/java/com/linecorp/armeria/internal/server/thrift/ThriftMetadataAccess.java

+
+ private static boolean preInitializeThriftClass;
+
+ private static final String filename = "../../common/thrift/thrift-options.properties";


Use an absolute path from the root resources?

Give a more meaningful name?

Suggested change

private static final String filename = "../../common/thrift/thrift-options.properties";

private static final String THRIFT_OPTION_FILE = "../../common/thrift/thrift-options.properties";

Use an absolute path from the root resources?

The path was changed to a relative path from this comment. #4688 (comment)

I think it makes sense that if anyone chooses to shade armeria, the option will still work as long as the directory structure doesn't change.

Give a more meaningful name?

Done 👍

....13/src/main/resources/com/linecorp/armeria/internal/common/thrift/thrift-options.properties

ikhoon

Thanks! 💯 👍 @jrhee17

ikhoon · 2023-03-24T08:00:53Z

The CI builds failed.

minwoox

Looks good, thanks a lot!
I also learned a lot from the discussion.

trustin

Thanks for your patience, @jrhee17! 🙇

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.

…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.<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.

add a failing test

64b0575

jrhee17 added the defect label Feb 21, 2023

jrhee17 added this to the 1.23.0 milestone Feb 21, 2023

jrhee17 added 7 commits February 22, 2023 09:48

fix lint failures

11ea0a4

clean up deadlocked tasks on test failure

06bff92

add a comment on cleanup

f95e1e8

pre-initialize classes for thrift modules under 0.14

44da42c

minor cleanups

a809fcb

fix a compilation error

3139112

fix lint

c29e06d

jrhee17 marked this pull request as ready for review February 22, 2023 06:47

jrhee17 requested review from ikhoon, minwoox and trustin as code owners February 22, 2023 06:47

jrhee17 changed the title ~~Fix an intermittent deadlock when DocService for thrift is loaded~~ Fix an intermittent deadlock when DocService is loaded for a Thrift service Feb 22, 2023

ikhoon reviewed Feb 28, 2023

View reviewed changes

address comments by @ikhoon

7865893

trustin reviewed Mar 7, 2023

View reviewed changes

jrhee17 added 7 commits March 8, 2023 16:29

use the Versions api

42bb808

increase number of iterations so a deadlock occurs more frequently

aef84ac

handle lint errors

916385c

lint

8dcd48f

address comments by @trustin

3d6f6d2

add missing resource copy

2ec578d

lint

fe618d0

trustin reviewed Mar 23, 2023

View reviewed changes

jrhee17 added 2 commits March 24, 2023 00:21

Merge branch 'main' of github.com:line/armeria into bugfix/thrift-doc…

fcb89c2

…-deadlock

address comments by @trustin

180517d

ikhoon reviewed Mar 23, 2023

View reviewed changes

address comments by @ikhoon

901f9ef

ikhoon approved these changes Mar 24, 2023

View reviewed changes

test failure

e9e82a7

minwoox approved these changes Mar 27, 2023

View reviewed changes

trustin approved these changes Mar 27, 2023

View reviewed changes

trustin merged commit be306ab into line:main Mar 27, 2023

ikhoon mentioned this pull request Mar 13, 2024

Fix to thread-safely load Thrift classes for old libthrift (< 0.9.3) #5497

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix an intermittent deadlock when `DocService` is loaded for a Thrift service #4688

Fix an intermittent deadlock when `DocService` is loaded for a Thrift service #4688

jrhee17 commented Feb 21, 2023 •

edited

Loading

ikhoon left a comment

ikhoon Feb 28, 2023

ikhoon Feb 28, 2023

ikhoon Feb 28, 2023

ikhoon Feb 28, 2023

trustin Mar 7, 2023 •

edited

Loading

trustin Mar 7, 2023

jrhee17 Mar 8, 2023 •

edited

Loading

trustin Mar 8, 2023

jrhee17 Mar 8, 2023

trustin Mar 8, 2023

trustin Mar 8, 2023

jrhee17 Mar 9, 2023

ikhoon Mar 23, 2023

jrhee17 Mar 24, 2023

ikhoon left a comment

ikhoon commented Mar 24, 2023

minwoox left a comment

trustin left a comment

	logger.trace("Unexpected exception while determining the 'armeria-thrift' version: ", e);
	logger.debug("Unexpected exception while determining the 'armeria-thrift' version: ", e);

	static boolean needPreInitialize(Properties props) {
	static boolean needsPreInitialization(Properties props) {


		import com.google.common.annotations.VisibleForTesting;

		public final class ThriftMetadataAccess {

	public final class ThriftMetadataAccess {
	final class ThriftMetadataAccess {

	public Class<?> loadClass(String name) throws ClassNotFoundException {
	public synchronized Class<?> loadClass(String name) throws ClassNotFoundException {


		private static boolean preInitializeThriftClass;

		private static final String filename = "../../common/thrift/thrift-options.properties";

	private static final String filename = "../../common/thrift/thrift-options.properties";
	private static final String THRIFT_OPTION_FILE = "../../common/thrift/thrift-options.properties";

Fix an intermittent deadlock when DocService is loaded for a Thrift service #4688

Fix an intermittent deadlock when DocService is loaded for a Thrift service #4688

Conversation

jrhee17 commented Feb 21, 2023 • edited Loading

ikhoon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

trustin Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrhee17 Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ikhoon left a comment

Choose a reason for hiding this comment

ikhoon commented Mar 24, 2023

minwoox left a comment

Choose a reason for hiding this comment

trustin left a comment

Choose a reason for hiding this comment

Fix an intermittent deadlock when `DocService` is loaded for a Thrift service #4688

Fix an intermittent deadlock when `DocService` is loaded for a Thrift service #4688

jrhee17 commented Feb 21, 2023 •

edited

Loading

trustin Mar 7, 2023 •

edited

Loading

jrhee17 Mar 8, 2023 •

edited

Loading