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

Serialization issues #223

Closed
vyazelenko opened this issue Oct 2, 2020 · 9 comments
Closed

Serialization issues #223

vyazelenko opened this issue Oct 2, 2020 · 9 comments

Comments

@vyazelenko
Copy link
Contributor

vyazelenko commented Oct 2, 2020

I've found several issues with how Serializable is used in the project:

  • No classes that are Serializable declare serialVersionUID fields (e.g. AsciiNumberFormatException, Int2ObjectHashMap etc.).

    The classes should declare serialVersionUID field but it must be compatible with the old code, i.e. the value of the serialVersionUID field must match values at least from the latest released versions of Agrona on JDK 8 to avoid breaking existing code. This needs to also work for generated classes.

  • Some serializable classes contain non-serializable fields:

    • Externally assigned fields - for example IntLruCache takes two parameters one java.util.function.IntFunction and another java.util.function.Consumer neither of which are serializable by default and assigns them to the non-transient fields.

      The easiest solution would be to mark the corresponding fields transient if it would not break the internal logic of the class. Otherwise the solution is require provided functions to be serializable. For example in case of IntLruCache the constructor signature will change to this:

           public <FACTORY extends IntFunction<E> & Serializable, CONSUMER extends Consumer<E> & Serializable> IntLruCache(
              @DoNotSub final int capacity,
              final FACTORY factory,
              final CONSUMER closer)
           {
              ...
           }

      Or maybe simple instanceof Serializable check when creating object would suffice.

    • Internal fields - for example Int2ObjectCache caches KeySet, ValueCollection and EntrySet none of which are serializable. In this case we can either mark fields as transient or make the corresponding classes serializable.

@mjpt777
Copy link
Contributor

mjpt777 commented Oct 2, 2020

I was against added Serializable but some people convince me it was just for testing. We should not need to be backwards compatible between versions with a serialVersionUID change.

@vyazelenko
Copy link
Contributor Author

@mjpt777 I guess we keep non-serializable lambdas as is, i.e. they render the corresponding collections non-serializable despite those implementing Serializable interface.

Possible solutions:

  • Drop Serializable from the collections that accept lambda as the constructor argument.
  • Add run-time check to ensure that provided lambda is serializable. Will break all existing code!
  • Add compile-time check check to ensure that provided lambda is serializable. Will break all existing code!
  • Add run-time check when serializing to ensure that use gets a nicer exception.
  • Do nothing - let it burn when someone tries to serialize.

@mjpt777
Copy link
Contributor

mjpt777 commented Oct 2, 2020

If things cannot be seralized then we should remove it. Which classes are impacted?

@vyazelenko
Copy link
Contributor Author

@mjpt777 Updated #224 to use serialVersionUID instead of @SuppressWarnings("serial").

@vyazelenko
Copy link
Contributor Author

Impacted classes:

  • org.agrona.collections.Int2ObjectCache
  • org.agrona.collections.IntLruCache
  • org.agrona.collections.UnmodifiableCollectionView

And the generated classes from them.

@mjpt777
Copy link
Contributor

mjpt777 commented Oct 2, 2020

We can remove Serializable from them.

@vyazelenko
Copy link
Contributor Author

Ok, will do in my PR/

@vyazelenko
Copy link
Contributor Author

The rest of this issues should be fixed with #224.

@mjpt777 mjpt777 closed this as completed Oct 2, 2020
@guidomedina
Copy link

I'm too late to this discussion but the reason there were Serializable was because of this issue #110

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

No branches or pull requests

3 participants