ISerializable cleanup #20220

morganbr · 2017-05-24T05:09:37Z

Changes to throw PlatformNotSupportedException from ISerializable.GetObjectData and serialization constructors on non-Serializable types. Also removes private serialization constructors and some unneeded code that was used to support serializing non-serializable types. A few tests testing GetObjectData implementations are also removed.

This is progress toward #19119.

…ObjectData and serialization constructors on non-serializable types. Also removes private serialization constructors and some unneeded code that was used to support serializing non-serializable types. A few tests testing GetObjectData implementations are also removed.

morganbr · 2017-05-24T05:10:28Z

CC @danmosemsft @dotnet/corert-contrib

danmoseley · 2017-05-24T05:22:56Z

It would be nice to have a message in the PNSE but I guess that would mean editing lots of resx's...

morganbr · 2017-05-24T05:26:49Z

@danmosemsft, that was roughly my thinking. There's really no reason to directly call GetObjectData or serialization constructors directly, so hopefully the standard message isn't too bad.

morganbr · 2017-05-24T05:27:58Z

I should have also clarified that if you actually try to (de)serialize the types, BinaryFormatter will throw a SerializationException with a message that says the type isn't serializable and won't try to call GetObjectData/serialization constructors.

danmoseley · 2017-05-24T05:29:23Z

Sounds good.

stephentoub · 2017-05-24T12:24:46Z

...ystem.Collections.Specialized/src/System/Collections/Specialized/NameObjectCollectionBase.cs

@@ -90,56 +90,12 @@ protected NameObjectCollectionBase(int capacity)

        protected NameObjectCollectionBase(SerializationInfo info, StreamingContext context)
        {
-            _serializationInfo = info;
+            throw new PlatformNotSupportedException();


I assume your tool to analyze common serialized types factored in base types? I'm wondering about how common it is to serialize a type derived from NameObjectCollectionBase.

I don't have (and can't quickly get) base type data. However, I did a quick GitHub search and found one instance of a serializable derived type. Additionally, since this is more or less a collection, it fits our general model, so I've added it back.

stephentoub · 2017-05-24T12:27:13Z

src/System.Collections/src/System/Collections/Generic/SortedSet.TreeSubSet.cs

@@ -45,12 +45,6 @@ public TreeSubSet(SortedSet<T> Underlying, T Min, T Max, bool lowerBoundActive,
                VersionCheckImpl();
            }

-            private TreeSubSet(SerializationInfo info, StreamingContext context)


Hmm. TreeSubSet is a type derived from SortedSet and is returned from methods on SortedSet like GetViewBetween. As far as the consumer is concerned, it's a SortedSet... seems odd that some SortedSets from the framework could be serialized and others couldn't, depending on from which method you got them. Maybe it should be on the list?

There are a ton of internal types like TreeSubSet (various KeyCollections, ValueCollections, Synchronized* etc). Do you think they all belong? Is TreeSubSet special in some way?

Expanding on that a little, I'd think having these sorts of types be serializable limits our internal implementation much more than the restrictions on field names. For example, lots of the older collections have various inner collections that could probably be replaced by generics.

How do you know from your analysis whether the SortedSets people were serializing were TreeSubSet or not? We could start by not having them and only later opting them in if really important, it's just very difficult to reason about. "You can serialize SortedSet... but only if you didn't get it from these APIs on SortedSet."

I agree we don't have that data. As a proxy, how's this? It looks like at least on NetFX, the only way to get a TreeSubSet is through GetViewBetween. According to apisof.net, GetViewBetween has very little usage. Additionally, serializing SortedSets is pretty rare compared to other concrete collections. The intersection of those two very small percentages is hopefully ~zero.

stephentoub · 2017-05-24T12:28:36Z

src/System.ComponentModel.TypeConverter/src/System/ComponentModel/LicenseException.cs

-            info.AddValue("instance", _instance);
-
-            base.GetObjectData(info, context);
+            throw new PlatformNotSupportedException();


There seems to be some inconsistency here. If the Exception-derived type was already overriding GetObjectData, then calling it will throw a PNSE, but if it wasn't already overriding GetObjectData, it won't throw?

stephentoub

Generally looks good. Left a few comments. I'm not sure about throwing PNSE from overrides of Exception's GetObjectData; maybe we just leave those as nops, so that there's consistency with Exception-derived types that don't have additional state and thus don't override GetObjectData. Throwing from exposed deserialization ctors probably still makes sense.

jimcarley · 2017-05-24T14:11:55Z

src/System.Transactions.Local/src/System/Transactions/TransactionState.cs

@@ -263,7 +263,7 @@ internal virtual void AddOutcomeRegistrant(InternalTransaction tx, TransactionCo

        internal virtual void GetObjectData(InternalTransaction tx, SerializationInfo serializationInfo, StreamingContext context)
        {
-            throw TransactionException.CreateTransactionStateException(tx._innerException, tx.DistributedTxId);
+            throw new PlatformNotSupportedException();


Please don't change this. If this "base" method executes, it is an invalid state error. That won't change even when we end up supporting serialization.

jimcarley · 2017-05-24T14:14:18Z

src/System.Transactions.Local/src/System/Transactions/TransactionState.cs

@@ -1533,7 +1516,7 @@ internal override void CreateAbortingClone(InternalTransaction tx)

        internal override void GetObjectData(InternalTransaction tx, SerializationInfo serializationInfo, StreamingContext context)
        {
-            throw CreateTransactionAbortedException(tx);
+            throw new PlatformNotSupportedException();


It is unfortunate that we would need to put this exception back when we start supporting serialization. Maybe comment out the old code and add the PlatformNotSupportedException so we don't need to go back and look for what the code used to be?

I missed that the methods in this file aren't ISerializable implementations. I've put their implementations back.

jimcarley · 2017-05-24T14:16:15Z

src/System.Transactions.Local/src/System/Transactions/TransactionState.cs

@@ -1683,7 +1666,7 @@ internal override void CheckForFinishedTransaction(InternalTransaction tx)

        internal override void GetObjectData(InternalTransaction tx, SerializationInfo serializationInfo, StreamingContext context)
        {
-            throw TransactionInDoubtException.Create(TraceSourceType.TraceSourceBase, SR.TransactionIndoubt, tx._innerException, tx.DistributedTxId);
+            throw new PlatformNotSupportedException();


Same comment about preserving previous code in a comment. It really applies to all the code that you have replaced with a PlatformNotSupportedException in System.Transactions.

zhenlan · 2017-05-24T22:09:06Z

cc: @shmao

morganbr · 2017-05-24T22:46:46Z

@jimcarley , just so we have a better understanding, what's your plan on serialization in Transactions? More detail on the intention of these changes is in #19119 , but we don't want every single type that was serializable on NetFX to also be serializable on Core. Rather, we just want types that are both:

Types that can be correctly serialized cross runtime/version/OS.
Types that we have data showing users would actually serialize them.

The data I collected for this change showed those that serializing Transactions is very rare (though it may happen).

jimcarley · 2017-05-24T22:59:28Z

@morganbr You are correct that there are probably very few users that serialize Transaction objects. In Desktop, it is a way to get a transaction to flow from one process/AppDomain to another. There are other ways to do it by using the TransactionInterop methods in both processes. But Serialization seemed like a more "managed-friendly" way to do it.

Transaction serialization requires distributed transaction support, thus the feature is currently not supported in Core. When a Tx is serialized, it is assumed that the intent is to give the transaction to another process/AppDomain. So we have to promote the transaction to become distributed because when the other process/AppDomain de-serializes the transaction, we now have two "participants" in the transaction and that requires a common commit coordinator, thus the need for a distributed transaction.

Yes. SysTx on Core currently doesn't have functional parity with Desktop. But once we do implement distributed transactions, it could have parity. By removing serialization of transactions, you are removing that potential for functional parity.

morganbr · 2017-05-24T23:04:08Z

@jimcarley thanks for the explanation. You're certainly free to add serialization back when you support distributed transactions (with the requisite compatibility and maintenance burdens).

shmao · 2017-05-24T23:09:37Z

src/System.Net.Mail/tests/Functional/SmtpExceptionTest.cs

-        public void TestConstructorWithSerialization()
-        {
-            string msg = "MESSAGE";
-            Exception inner = new ArgumentException("whatever");


Question: would ArgumentException still be [Serializable] and implementing ISerializable?

We have tests for DataContractSerializer serializing ArgumentException. I wondered if it's still supported.

ArgumentException isn't marked Serializable, but that test passes. Perhaps DCS is using POCO serialization with it?

Hmm, interesting.. I just found that you've already removed [Serializable] from ArgumentException . I'm not sure why test test still passes. But I think it would fail if we change its GetObjectData method.

… This makes them behave consistently with exceptions that didn't override GetObjectData.

morganbr · 2017-05-25T00:48:15Z

@stephentoub I agree that throwing from GetObjectData on some exceptions, but not others is problematic. I've switched all of the overrides to simply call base instead.

shmao · 2017-05-25T00:59:35Z

@morganbr If a type is [Serializable] and ISerializable and it already has implementation for ISerializable, e.g. ArgumentException, do we still want to make changes to the type?

morganbr · 2017-05-25T01:19:03Z

@shmao in general, yes we are removing [Serializable] and throwing from ISerializable on types that aren't on the list regardless of whether they have implementations. The full reasoning is in #19119, but essentially, each [Serializable] type has an additional compatibility cost and many of them bring in other types. The list is based on data we've gathered on serialization usage along with some obvious expansions (e.g. people only serialize Tuple`2-6 in our data, but we should include the rest of the Tuples too).

That said, we're certainly open to adding individual types if they clearly add user value. Feel free to file issues on types you believe are missing that you have evidence will be serialized.

morganbr · 2017-05-25T03:03:09Z

@dotnet-bot test Innerloop netfx Windows_NT Debug x86 Build and Test dotnet/roslyn#6358
@dotnet-bot test Portable Linux x64 Release Build
@dotnet-bot test Win x64 tests - Debug - Windows.10.Nano.Amd64.Open #20283
@dotnet-bot test Win x64 tests - Release - Windows.10.Nano.Amd64.Open #20283

morganbr · 2017-05-25T03:26:53Z

@dotnet-bot test Innerloop Ubuntu14.04 Debug x64 Build and Test
@dotnet-bot test Portable Linux x64 Release Build
@dotnet-bot test Win x64 tests - Debug - Windows.10.Nano.Amd64.Open
@dotnet-bot test Win x64 tests - Release - Windows.10.Nano.Amd64.Open

morganbr · 2017-05-25T03:27:14Z

@dotnet-bot test Portable Linux x64 Release Build

morganbr · 2017-05-25T03:28:00Z

I'm unclear on why those legs aren't restarting, but I've verified the failures aren't related to this change.

danmoseley · 2017-05-25T04:23:04Z

@dotnet-bot test Portable Linux x64 Release Build please

danmoseley · 2017-05-25T04:23:37Z

@mmitche it won't restart portable leg.

danmoseley · 2017-05-25T04:24:49Z

You can ignore the Nano failures. I fixed them.

danmoseley · 2017-05-25T04:25:53Z

@mmitche also what does it mean that Linux x64 Tests - Release - RedHat.73.Amd64.Open is hung and where can I see its results so far?
also there are some nasty looking callstack at the end of https://ci3.dot.net/job/dotnet_corefx/job/master/job/portable-linux-Config_Release+OuterLoop_false_prtest/14/console

danmoseley · 2017-05-25T19:38:05Z

Stephen signed off.

mmitche · 2017-05-25T19:54:19Z

@danmosemsft Different trigger phrase. @dotnet-bot test portable linux debug pipeline. Want me to change the trigger phrases to be the same as the context?

* Changes to throw PlatformNotSupportedException from ISerializable.GetObjectData and serialization constructors on non-serializable types. Also removes private serialization constructors and some unneeded code that was used to support serializing non-serializable types. A few tests testing GetObjectData implementations are also removed. * Address code review comments. * Change exceptions' GetObjectData to just call base rather than throw. This makes them behave consistently with exceptions that didn't override GetObjectData.

mgravell · 2019-05-22T13:57:12Z

Can I ask a question on this? I found this today, when a DataSet failed to serialize correctly when RemotingFormat is SerializationFormat.Binary, because System.Data.SimpleType isn't [Serializable]. Is there a preferred route forward here? or is it just "sometimes, DataSet won't serialize"?

danmoseley · 2019-05-22T17:09:17Z

@mgravell As you know BinaryFormatter is a dead-end technology for us because of maintenance and security reasons. We limited [Serializable] to a small list of exceptions, collections, and primitive types. DataSet somehow made it in there. I don't know whether SimpleType was an oversight. I recall we overlooked DBNull as well. The serialization for this one requires ISerializable which makes it a bit more complicated/fragile.

Stepping back, can you use a different serialization mechanism? This probably won't be the only way DataSet can fail to serialize this way.

mgravell · 2019-05-22T18:43:48Z

that's fair, @danmosemsft ; for now, I'm removing the SerializationFormat.Binary in this scenario, which ... makes it bigger and slower, but it works. But yeah, I totally hear you on BinaryFormatter in general. Thanks for the input.

morganbr self-assigned this May 24, 2017

morganbr requested review from stephentoub, jkotas, ViktorHofer and zhenlan May 24, 2017 05:09

dnfclas added the cla-already-signed label May 24, 2017

morganbr added this to the 2.1.0 milestone May 24, 2017

stephentoub reviewed May 24, 2017

View reviewed changes

jimcarley reviewed May 24, 2017

View reviewed changes

shmao reviewed May 24, 2017

View reviewed changes

morganbr added 2 commits May 24, 2017 16:58

Address code review comments.

932bec8

Change exceptions' GetObjectData to just call base rather than throw.…

487a93c

… This makes them behave consistently with exceptions that didn't override GetObjectData.

danmoseley closed this May 25, 2017

danmoseley reopened this May 25, 2017

dnfclas added the cla-already-signed label May 25, 2017

danmoseley merged commit a82f54b into dotnet:master May 25, 2017

morganbr mentioned this pull request May 26, 2017

ISerializable cleanup dotnet/coreclr#11873

Merged

pranavkm mentioned this pull request Jun 5, 2017

Mock.Of<T> fails with exception 'Unhandled Exception: System.ArgumentException: Interface not found' devlooped/moq#340

Closed

morganbr deleted the TrimISerializable branch June 7, 2017 01:13

Cisien mentioned this pull request Aug 19, 2017

PlatformNotSupportedException when serializing some ISerializable types on dotnet core 2.0 JamesNK/Newtonsoft.Json#1404

Closed

Cisien mentioned this pull request Jan 31, 2020

PlatformNotSupportedException when attempting to serialize DirectoryInfo instance with Newtonsoft.Json dotnet/runtime#23261

Closed

ISerializable cleanup #20220

ISerializable cleanup #20220

Conversation

morganbr commented May 24, 2017

morganbr commented May 24, 2017

danmoseley commented May 24, 2017

morganbr commented May 24, 2017

morganbr commented May 24, 2017

danmoseley commented May 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stephentoub May 24, 2017 • edited Loading

Choose a reason for hiding this comment

stephentoub left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zhenlan commented May 24, 2017

morganbr commented May 24, 2017

jimcarley commented May 24, 2017

morganbr commented May 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

morganbr commented May 25, 2017

shmao commented May 25, 2017

morganbr commented May 25, 2017

morganbr commented May 25, 2017

morganbr commented May 25, 2017

morganbr commented May 25, 2017

morganbr commented May 25, 2017

danmoseley commented May 25, 2017

danmoseley commented May 25, 2017

danmoseley commented May 25, 2017

danmoseley commented May 25, 2017 • edited Loading

danmoseley commented May 25, 2017

mmitche commented May 25, 2017

mgravell commented May 22, 2019

danmoseley commented May 22, 2019

mgravell commented May 22, 2019

stephentoub May 24, 2017 •

edited

Loading

danmoseley commented May 25, 2017 •

edited

Loading