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

Fix race for concurrent serialization of recursive type #767

Closed
wants to merge 1 commit into from
Closed

Fix race for concurrent serialization of recursive type #767

wants to merge 1 commit into from

Conversation

pietern
Copy link

@pietern pietern commented Jan 7, 2016

This race happens when two or more threads try to serialize the same recursive type for the first time at the same time.

The fix makes sure that TypeAdapter instances are not cached in the instance cache until all FutureTypeAdapters in a re-entrant call to getAdapter() have their delegates set.

Also see: #764

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed, please reply here (e.g. I signed it!) and we'll verify. Thanks.


  • If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
  • If you signed the CLA as a corporation, please let us know the company's name.

@pietern
Copy link
Author

pietern commented Jan 7, 2016

This commit breaks a couple of tests, so please don't consider this final.

Pushed to have a starting point.

@swankjesse
Copy link
Collaborator

My guess is that the best fix will be to change FutureTypeAdapter to look up the type adapter in Gson when it suffers a miss.

@pietern
Copy link
Author

pietern commented Jan 7, 2016

@swankjesse I think that would be quite an elaborate fix since you would have to synchronize between the two threads. Add a latch so that the thread that misses can wait on the one that sets the delegate.

This race happens when two or more threads try to serialize the same
recursive type for the first time at the same time.

The fix makes sure that TypeAdapter instances are not cached in the
instance cache until all FutureTypeAdapters in a re-entrant call to
getAdapter() have their delegates set.

Also see: #764
@pietern
Copy link
Author

pietern commented Jan 7, 2016

Changed the fix to overwrite the future type adapters with the real one in the threadCalls map. This leads to the same result as before, where the newly built types are only externalized to other threads once the last one has been created, unlike the situation before this fix.

@pietern
Copy link
Author

pietern commented Jan 19, 2016

The CLA is taken care of. See the vmware-google-contributors Google Group.

@swankjesse This is ready to merge, if this is the way you want to go for the fix (it's the most straightforward way to fix it in my opinion, by not leaking any adapters into the cache before the whole tree is traversed).

@ttddyy
Copy link

ttddyy commented Jun 6, 2016

Hi there,

I was looking #764 and came this PR.
Is this fix going to be merged, or already solved by other way?

Thanks,

@itsgrimetime
Copy link

Ping. Any chance of this getting merged in?

@yrodiere
Copy link

yrodiere commented Oct 3, 2019

Hello,

The fix looks good, any particular reason to delay merging?

@Marcono1234
Copy link
Collaborator

Thanks a lot for you work on this! This has now be fixed by #1832, but thank you nonetheless!

@Marcono1234 Marcono1234 closed this Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants