The Wayback Machine - https://web.archive.org/web/20221117144824/https://github.com/pythonnet/pythonnet/pull/1892
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

Best effort serialization #1892

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

BadSingleton
Copy link
Contributor

@BadSingleton BadSingleton commented Jul 29, 2022

What does this implement/fix? Explain your changes.

This PR adds a "last line of defense, best effort serialization" to serialize types not marked as Serializable. Such objects, when derivable, are deserialized as derived classes with all (overridable) methods and properties overriden to throw a "Not Serialized" Exception. Fields are not initialized and may be null. Instances of classes that are not derivable are null.

Does this close any currently open issues?

No, but it is an issue we've encountered with Unity.

Any other comments?

This is a "best effort" to ensure no SerializationExceptions are thrown on domain unloads/reloads. This should help users deal with serialization issues as sometimes unserializable classes may be in compiled libraries and therefore unmodifiable.

Checklist

Check all those that are applicable and complete.

  • Make sure to include one or more tests for your change
  • If an enhancement PR, please create docs and at best an example
  • Ensure you have signed the .NET Foundation CLA
  • Add yourself to AUTHORS
  • Updated the CHANGELOG

This commit adds a "last line of defense, best effort serialization"
to serialize types not marked as Serializable. Such objects are
deserialized as derived classes with all methods and properties
overriden to throw a "Not Serialized" Exception.
Fields are not initialized and may be null.

Sealed classes and implemented interface methods are still a problem to
be solved.
plus guard rails for private classes

and review fixes
don't try to derive from sealed types
@BadSingleton
Copy link
Contributor Author

BadSingleton commented Jul 29, 2022

Also just a note that I'll soon be away for vacations until october, I just want to get that ball rolling and gather feedback in the meantime.

@lostmsu
Copy link
Member

lostmsu commented Jul 29, 2022

@BadSingleton is there a way to provide that functionality without bringing the NonSerializedTypeBuilder into Python.NET? E.g. can we allow user to override the serializer somehow, and you guys would be able to keep this code inside Unity or as a new subproject like pythonnet/unity-serializer?

@lostmsu
Copy link
Member

lostmsu commented Jul 29, 2022

Also, can you describe the approach in more details?

@BadSingleton
Copy link
Contributor Author

BadSingleton commented Jul 29, 2022

@lostmsu I think the serializer was meant to be overridable in the first implementations of the serialization. I think I recall @amos402 saying something about the need for users to implement their own. If it becomes overridable we probably can keep it outside of this repo (just need to make sure with some order of initialization things)

The idea was to ensure that the presence of a non-serializable object in the graph wouldn't throw an exception, therefore corrupting the whole shutdown process and causing crashes on domain (un)load. As sometimes those objects are outside of our control, we can't modify them and/or avoid easily to serialize them (and/or the users of our APIs may be passing a Unity object that is not serializable). After a few google searches I found out that serialization surrogates could be used to serialize objects not marked [Serializable]. Since these objects are not meant to be serialized, but they're in the object graph that is being serialized, we serialize the type, not the object. When deserializing, there are two possibilities:

  1. The object can be derived from, and we add _NotSerialized suffix to the class name. If it is, every method and property (get and set) are overridden by a simple function that throws a NotSerializedException (the inspiration for the exception message comes from PyQt. Those who knows, know the pain.). Fields are left null.
  2. The object cannot be derived from (private, internal, ...). Not much to do here, except returning a null reference.

When serializing a derived _NotSerialized class, we serialize the base class to avoid having _NotSerialized_NotSerialized_... looping.

@BadSingleton
Copy link
Contributor Author

BadSingleton commented Jul 29, 2022

Ignore the part of the previous comment about the serializer not being overridable, I misremembered the code, it is overridable.

@lostmsu
Copy link
Member

lostmsu commented Jul 29, 2022

It seems that this code can be abstracted out of this repo with:

  1. Having a public post stash and pre restore hooks
  2. Exposing a wrapper for PyCapsule and maybe PyMem (though I think it can be replaced with Marshal.AllocHGlobal)
  3. Replacing FormatterType property with FormatterFactory
  4. Exposing MaybeType

@BadSingleton
Copy link
Contributor Author

BadSingleton commented Jul 29, 2022

Worst case, MaybeType is easily re-implementable. I'm not sure about exposing it.

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.

None yet

2 participants