-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix non-threadsafe creation of adapter for type with cyclic dependency #1832
Fix non-threadsafe creation of adapter for type with cyclic dependency #1832
Conversation
2c79905
to
64cbd30
Compare
64cbd30
to
9408145
Compare
Any plan to merge this? We would really like to see this fixed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
* 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 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
@@ -161,8 +162,8 @@ public final class Gson { | |||
* lookup would stack overflow. We cheat by returning a proxy type adapter. | |||
* The proxy is wired up once the initial adapter has been created. | |||
*/ | |||
private final ThreadLocal<Map<TypeToken<?>, FutureTypeAdapter<?>>> calls | |||
= new ThreadLocal<>(); | |||
// Uses LinkedHashMap because iteration order is important, see getAdapter() implementation below |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 aTypeAdapter
, and the corresponding value is either a successfulTypeAdapter
or a proxy for a futureTypeAdapter
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have reverted usage of LinkedHashMap
as part of simplifying this pull request. I hope the newly adjusted doc comment is fine.
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
return ongoingCall; | ||
threadLocalAdapterResults.set(threadCalls); | ||
isInitialAdapterRequest = true; | ||
} else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
} | ||
} | ||
throw new IllegalArgumentException("GSON (" + GsonBuildConfig.VERSION + ") cannot handle " + type); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have moved this and the return
outside the try
-finally
.
Thanks, this looks great! |
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! |
The just-released 2.10.1 includes this fix. |
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)
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.