Skip to content
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

I don't quite understand why getAdapter() method is thread safe? #1509

Closed
macarthor opened this issue Apr 9, 2019 · 3 comments
Closed

I don't quite understand why getAdapter() method is thread safe? #1509

macarthor opened this issue Apr 9, 2019 · 3 comments
Labels

Comments

@macarthor
Copy link

quote a part of getAdapter() method:

private final Map<TypeToken<?>, TypeAdapter<?>> typeTokenCache = new ConcurrentHashMap<TypeToken<?>, TypeAdapter<?>>();

public <T> TypeAdapter<T> getAdapter(TypeToken<T> type) {
    TypeAdapter<?> cached = typeTokenCache.get(type == null ? NULL_KEY_SURROGATE : type);
    if (cached != null) {
      return (TypeAdapter<T>) cached;
    }
    ......
    for (TypeAdapterFactory factory : factories) {
      TypeAdapter<T> candidate = factory.create(this, type);
      if (candidate != null) {
        call.setDelegate(candidate);
        typeTokenCache.put(type, candidate);
        return candidate;
      }
    }
    ......
}

typeTokenCache.get and typeTokenCache.put are not an atomic pair. So why this method, as well as fromJson()s and toJson()s referencing it, is thread safe?

@JakeWharton
Copy link
Contributor

That just means that work is duplicated when an adapter is requested for the first time concurrently. Creating the same adapter multiple times should always produce the same result.

@macarthor
Copy link
Author

macarthor commented Apr 10, 2019

@JakeWharton I'd try to explain my current understanding.

  • suppose there are concurrent calls to getAdapter(TypeToken<T> type), with multiple instances of TypeToken<T>. each instance has the same T class.
  • a TypeAdapterFactory instance is selected, and create a TypeAdapter<T> instance in each thread. it seems that there may be multiple TypeAdapter<T> instances created.
  • actually, only ONE TypeAdapter<T> instance is created in each thread. Reason 1: existing TypeAdapterFactory implementations create TypeAdapter<T> instance not by TypeToken<T>, but its final Class<? super T> rawType member. As we know, Class has one instance for a given ClassLoader. Reason 2: in TypeAdapterFactory JavaDoc, it says If multiple factories support the same type, the factory registered earlier takes precedence. NOTE: the same type represents T instead of TypeAdapter<T>, i believe. This is somewhat implicit, isn't it...
  • those instances would try to be put into map by typeTokenCache.put(type, candidate).
  • typeTokenCache would have multiple keys corresponding to one value.
  • next calls to ``typeTokenCache.get()is would get oneTypeAdapter` instance, even if you create another instance of `TypeToken`. what's different is you add another key into `typeTokenCache` with the same value.
  • now, the getAdapter() method guarantees that for different instances of TypeToken<T>, you get the same TypeAdapter<T> instance as long as they share the same T. from this point of view, the method is thread safe.

am i right? is there any mistake in my understanding?

@Marcono1234
Copy link
Collaborator

Marcono1234 commented Aug 16, 2022

As already mentioned above in #1509 (comment) even if a TypeAdapterFactory always creates new TypeAdapter instances, they should behave identical if the arguments to TypeAdapterFactory.create were equal (if I understand the comment correctly).

The previous Gson code using typeTokenCache.put might indeed have been a bit problematic if a caller of getAdapter(...) expected that exactly the same instance is returned (though you should probably avoid relying on this).
In #2153 the code was refactored to use typeTokenCache.putIfAbsent. Therefore getAdapter(...) will now always return exactly the same adapter instance, even when it is called concurrently by multiple threads.

I am closing this issue because I think this has been answered. If you have further questions, feel free to ask.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants