Fix non-threadsafe creation of adapter for type with cyclic dependency #1832

Marcono1234 · 2020-12-01T23:40:11Z

Fixes #625

#625 (comment) and #764 (comment) describe the issue very well.

The author of #1830 was slightly faster in providing a fix for this issue, though this pull request here also improves the exception messages.
There is also #767 which might fix this issue as well.

byencho · 2022-11-22T15:01:28Z

Any plan to merge this? We would really like to see this fixed.

Marcono1234

@eamonnmcmanus could you please have a look?

Unfortunately these changes are a bit complex (both in Gson and GsonTest). A simpler solution would be possible see #767 or #1830, at the cost of producing less helpful exception messages when TypeAdapterFactory implementations discard exceptions thrown by getAdapter and fall back to requesting the adapter for a different type (note that the tests here cover that scenario). Though on the other hand this might be a pretty rare corner case which does not justify this complexity.
If you want I can remove the handling for that case.

gson/src/main/java/com/google/gson/Gson.java

Marcono1234 · 2022-11-23T01:13:18Z

gson/src/test/java/com/google/gson/GsonTest.java

+   * and lets one thread wait after the adapter for CustomClassB1 has been obtained (which still
+   * contains the nested unresolved FutureTypeAdapter for CustomClassA).
+   */
+  public void testGetAdapterFutureAdapterConcurrency() throws Exception {


This test is quite verbose but it should hopefully cover exactly the situation #625 is about; reverting the Gson changes of this pull request but keeping these GsonTest changes triggers the bug described in that issue (IllegalStateException being thrown).

eamonnmcmanus

Thanks for all the work that has gone into this!

I'm concerned that the logic here is quite hard to understand, and may be trying to solve problems that we could get away with not solving. Perhaps we can simplify, while still fixing #625?

eamonnmcmanus · 2022-11-28T21:45:33Z

gson/src/main/java/com/google/gson/Gson.java

   */
-  private final ThreadLocal<Map<TypeToken<?>, FutureTypeAdapter<?>>> calls
-      = new ThreadLocal<>();
+  // Uses LinkedHashMap because iteration order is important, see getAdapter() implementation below


I think this could reasonably be inside the doc comment, especially since the field is private.

Can you also say exactly what the map represents in the doc comment? Maybe something like this:

A key in the LinkedHashMap for a thread is a type for which that thread is currently making a TypeAdapter, and the corresponding value is either a successful TypeAdapter or a proxy for a future TypeAdapter.

Have reverted usage of LinkedHashMap as part of simplifying this pull request. I hope the newly adjusted doc comment is fine.

gson/src/main/java/com/google/gson/Gson.java

gson/src/test/java/com/google/gson/GsonTest.java

eamonnmcmanus · 2022-11-28T22:27:08Z

gson/src/main/java/com/google/gson/Gson.java

+            // Publish resolved adapters to all threads
+            // Can only do this for the initial request because cyclic dependency TypeA -> TypeB -> TypeA
+            // would otherwise publish adapter for TypeB which uses not yet resolved adapter for TypeA
+            // See https://github.com/google/gson/issues/625


Just so I understand, in a situation like this the second TypeA will get a FutureTypeAdapter, TypeB will reference that FutureTypeAdapter, then the first TypeA will get the actual TypeAdapter returned by the TypeAdapterFactory. TypeB will continue to reference the FutureTypeAdapter it got for TypeA, but its delegate will have been updated to be the actual TypeAdapter. Is that right?

Yes that is correct. And in the past the issue was that the adapter for TypeB (which references the FutureTypeAdapter) was already published to other threads before the FutureTypeAdapter had been resolved. That was not thread-safe.

Marcono1234

Thanks for the review! I have simplified this pull request now as discussed in the above review comments:

omitting the handling for the contrived case "exception from getAdapter being discarded"
omitting preserving existing adapter instance for concurrent getAdapter calls, since that behavior could not be guaranteed in all cases anymore

Marcono1234 · 2022-12-03T20:22:05Z

gson/src/main/java/com/google/gson/Gson.java

-      return ongoingCall;
+      threadLocalAdapterResults.set(threadCalls);
+      isInitialAdapterRequest = true;
+    } else {


Have moved the code here in the else block; otherwise the ongoingCall check will be unsuccessful since the if statement just created an empty map as threadCalls value.

Marcono1234 · 2022-12-03T20:22:48Z

gson/src/main/java/com/google/gson/Gson.java

+          break;
        }
      }
-      throw new IllegalArgumentException("GSON (" + GsonBuildConfig.VERSION + ") cannot handle " + type);


Have moved this and the return outside the try-finally.

eamonnmcmanus · 2022-12-05T17:10:31Z

Thanks, this looks great!

richardm-stripe · 2023-01-04T20:04:04Z

Hello @eamonnmcmanus! Will this be in GSON 2.11 and can you share when 2.11 is expected to land? Our users run into this sometimes, and we'd like to judge whether to wait for 2.11 or put a workaround for this into our library.

Thank you!

eamonnmcmanus · 2023-01-06T17:25:51Z

The just-released 2.10.1 includes this fix.

byencho · 2023-01-06T17:52:52Z

Thanks @eamonnmcmanus !

google#1832) * Fix non-threadsafe creation of adapter for type with cyclic dependency * Improve handling of broken adapters during Gson.getAdapter(...) call * Improve test * Slightly improve implementation and extend tests * Simplify getAdapter implementation * Convert GsonTest to JUnit 4 test * Clarify getAdapter concurrency behavior (cherry picked from commit e4c3b65)

google-cla bot added the cla: yes label Dec 1, 2020

Fix non-threadsafe creation of adapter for type with cyclic dependency

07b27f1

Marcono1234 force-pushed the marcono1234/threadsafe-cyclic-adapter branch from 2c79905 to 64cbd30 Compare December 1, 2020 23:46

Improve handling of broken adapters during Gson.getAdapter(...) call

9408145

Marcono1234 force-pushed the marcono1234/threadsafe-cyclic-adapter branch from 64cbd30 to 9408145 Compare August 12, 2021 23:05

Marcono1234 added 2 commits February 5, 2022 22:15

Merge branch 'master' into marcono1234/threadsafe-cyclic-adapter

eba98ca

Improve test

1bb129d

atlslscsrv-app approved these changes Sep 10, 2022

View reviewed changes

byencho mentioned this pull request Nov 18, 2022

FutureTypeAdapter.read() throws unhelpful exception #625

Closed

Marcono1234 added 2 commits November 23, 2022 01:29

Slightly improve implementation and extend tests

96e609b

Merge branch 'master' into marcono1234/threadsafe-cyclic-adapter

92e4f12

Marcono1234 commented Nov 23, 2022

View reviewed changes

eamonnmcmanus requested changes Nov 28, 2022

View reviewed changes

Marcono1234 added 3 commits December 3, 2022 21:15

Simplify getAdapter implementation

29a6969

Convert GsonTest to JUnit 4 test

8f2faa6

Clarify getAdapter concurrency behavior

a5ba266

Marcono1234 commented Dec 3, 2022

View reviewed changes

eamonnmcmanus approved these changes Dec 5, 2022

View reviewed changes

eamonnmcmanus merged commit e4c3b65 into google:master Dec 5, 2022

Marcono1234 deleted the marcono1234/threadsafe-cyclic-adapter branch December 5, 2022 21:20

apflieger mentioned this pull request Dec 5, 2022

Invalid response object from API stripe/stripe-java#1213

Closed

richardm-stripe mentioned this pull request Jan 11, 2023

Bump Gson to 2.10.1 stripe/stripe-java#1505

Merged

Fix non-threadsafe creation of adapter for type with cyclic dependency #1832

Fix non-threadsafe creation of adapter for type with cyclic dependency #1832

Uh oh!

Conversation

Marcono1234 commented Dec 1, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

byencho commented Nov 22, 2022

Uh oh!

Marcono1234 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Marcono1234 Nov 23, 2022

Choose a reason for hiding this comment

Uh oh!

eamonnmcmanus left a comment

Choose a reason for hiding this comment

Uh oh!

eamonnmcmanus Nov 28, 2022

Choose a reason for hiding this comment

Uh oh!

Marcono1234 Dec 3, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eamonnmcmanus Nov 28, 2022

Choose a reason for hiding this comment

Uh oh!

Marcono1234 Dec 3, 2022

Choose a reason for hiding this comment

Uh oh!

Marcono1234 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Marcono1234 Dec 3, 2022

Choose a reason for hiding this comment

Uh oh!

Marcono1234 Dec 3, 2022

Choose a reason for hiding this comment

Uh oh!

eamonnmcmanus commented Dec 5, 2022

Uh oh!

richardm-stripe commented Jan 4, 2023

Uh oh!

eamonnmcmanus commented Jan 6, 2023

Uh oh!

byencho commented Jan 6, 2023

Uh oh!

Uh oh!

Marcono1234 commented Dec 1, 2020 •

edited

Loading

Marcono1234 left a comment •

edited

Loading

Marcono1234 left a comment •

edited

Loading