Make corefx exceptions serializable and add typeforwards #24427

ViktorHofer · 2017-10-04T16:41:33Z

Related: https://github.com/dotnet/corefx/issues/24424

Exceptions that are partially supported:

LicenseException
ReflectionTypeLoadException
SecurityException
SqlException

Exceptions that aren't supported:

XamlParseException
InvalidPrinterException
XPathCompileException
CryptoSignedXmlRecursionException
ElementNotAvailableException
ElementNotEnabledException

ViktorHofer · 2017-10-04T17:34:17Z

@weshaggard ZLibException is internal in System.IO.Compression. Should we add InternalsVisible to that package or do we need to find another way? Maybe make the exception public only in the impl?

weshaggard · 2017-10-04T21:08:34Z

...icrosoft.CSharp/src/Microsoft/CSharp/RuntimeBinder/RuntimeBinderInternalCompilerException.cs

@@ -12,6 +12,8 @@ namespace Microsoft.CSharp.RuntimeBinder
    /// <see cref="RuntimeBinderException"/> represents a failure to bind in the sense of a usual compiler error, whereas <see cref="RuntimeBinderInternalCompilerException"/>
    /// represents a malfunctioning of the runtime binder itself.
    /// </summary>
+    [Serializable]
+    [System.Runtime.CompilerServices.TypeForwardedFrom("Microsoft.CSharp, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a")]


Is this needed? It is still in that same assembly.

good points. I will remove it

joperezr · 2017-10-04T22:01:16Z

src/shims/manual/mscorlib.forwards.cs

@@ -20,6 +18,8 @@
 [assembly:System.Runtime.CompilerServices.TypeForwardedTo(typeof(System.Collections.Generic.ShortEnumEqualityComparer<>))]
 [assembly:System.Runtime.CompilerServices.TypeForwardedTo(typeof(System.Collections.Generic.LongEnumEqualityComparer<>))]
 [assembly:System.Runtime.CompilerServices.TypeForwardedTo(typeof(System.Collections.ListDictionaryInternal))]
+[assembly:System.Runtime.CompilerServices.TypeForwardedTo(typeof(System.CultureAwareComparer))]


Why do we need to reorder the forwards?

We don't need and leave it as it is.

joperezr · 2017-10-04T22:04:51Z

src/System.ComponentModel.TypeConverter/src/System/ComponentModel/WarningException.cs

@@ -66,7 +68,8 @@ public WarningException(string message, string helpUrl, string helpTopic)
        /// </summary>
        protected WarningException(SerializationInfo info, StreamingContext context) : base(info, context)
        {
-            throw new PlatformNotSupportedException();
+            HelpUrl = (string)info.GetValue("helpUrl", typeof(string));


helpUrl [](start = 45, length = 7)

I suppose casing here is like that in order to match Desktop. Should we add a comment or something so that it doesn't get renamed?

Casing of the field or the string? The string matches Desktop, correct. The HelpUrl field/property is from the base type Exception.

I meant the string, helpUrl. I thought you were adding comments in other serializable types so that people won't rename strings since it would cause a compatibility with desktop.

Jep I will do that with another cleanup change that Stephen suggested (replacing the assemblyqualifiednames in the typeforwards with consts) in a separate PR after I finished the tests and hopefully merged it into the servicing release.

weshaggard · 2017-10-04T22:07:55Z

src/Microsoft.CSharp/tests/RuntimeBinderExceptionTests.cs

-        [Fact]
-        public void AssertExceptionDeserializationFails()
-        {
-            BinaryFormatterHelpers.AssertExceptionDeserializationFails<RuntimeBinderException>();


Should these be replaced with valid tests now?

It will be replaced with generic tests in System.Runtime.Serialization.Formatters.Tests that check for the correct ctor behavior

weshaggard · 2017-10-04T22:34:16Z

@weshaggard ZLibException is internal in System.IO.Compression. Should we add InternalsVisible to that package or do we need to find another way? Maybe make the exception public only in the impl?

Why are we making all the internal exceptions serializable? I'm not sure I like exposing them publicly.

danmoseley · 2017-10-04T23:54:52Z

Exceptions that aren't supported because they try to serialize non-serializable fields:
InvalidPrinterException
LicenseException
OdbcException
ReflectionTypeLoadException
SecurityException
SqlException

I think we can support at least some of these safely without making any new types serializable, so long as we can use defaults that the implementation would accept, and would not leave the objects in a bad state. (And what bad state is is not well defined, since this is serialization input that anyone could already give the types today -- in this context it probably means, the exception can be rethrown, and message/stack trace at least read off it.)

SecurityException we csan support by just using null for AssemblyName, and default value for SecurityAction and SecurityZone (we do not have those enums, but you can have dummy private ones with just a single value to serialize). In the SecurityException serialization constructor, it already falls back to null and defaults in the error path, so it implicitly considers this a valid payload even.

SqlException you just need to serialize null for "Errors". Again it looks like the code accepts null

ReflectionTypeLoadException it is fine for Types to be null: the regular constructor accepts it. Similar argument for LicenseException. Also for OdbcException, the regular constructor accepts null. You would need to fake ODBC32.RETCODE by having a dummy private enum.

InvalidPrinterExceptoin regular constructor throws on null, but nevertheless I think it would be reasonable to use.

Of thse I think we only care about Securityexception and SqlException and possibly ReflectionTypeLoadException .

danmoseley · 2017-10-04T23:57:15Z

Do you plan to include tests in this checkin? I suggest writing all the necessary tests before committing either corelib or corefx. Then commit corelib. Then roll the tests into this PR and they will pass when it picks up coreclr.

ViktorHofer · 2017-10-05T15:42:02Z

Why are we making all the internal exceptions serializable? I'm not sure I like exposing them publicly.

We try to support serialization of all exceptions. This includes internal ones...

ViktorHofer · 2017-10-05T15:45:54Z

Do you plan to include tests in this checkin?

Yes

ViktorHofer · 2017-10-09T16:52:40Z

Not ready for review yet

I'm currently hitting a bug when compiling the System.Runtime.Serialization.Formatters.Tests project and Wes is looking into it.

danmoseley · 2017-10-09T21:32:57Z

src/System.Runtime.Serialization.Formatters/tests/BinaryFormatterTests.cs

@@ -486,7 +486,7 @@ private static void SanityCheckBlob(object obj, string[] blobs)
            }

            // Exceptions in Net Native can't be reflected and therefore skipping blob sanity check
-            if (obj is Exception && PlatformDetection.IsNetNative)
+            if (obj is Exception/* && PlatformDetection.IsNetNative*/)


danmoseley · 2017-10-09T21:35:17Z

src/System.Net.Primitives/src/System/Net/SocketException.cs

@@ -43,7 +45,7 @@ internal SocketException(SocketError socketError) : base(GetNativeErrorForSocket
        protected SocketException(SerializationInfo serializationInfo, StreamingContext streamingContext)
            : base(serializationInfo, streamingContext)
        {
-            throw new PlatformNotSupportedException();
+            if (NetEventSource.IsEnabled) NetEventSource.Info(this, $"{NativeErrorCode}:{Message}");


this may be right, but it doesn't match desktop

ViktorHofer · 2017-10-13T16:59:48Z

All exceptions are now serializable from Desktop to Core except a few mentioned below. I had to use reflection for some. Note: I made OdbcErrorCollection serializable to support OdbcException. I think making that type serializable is safe as there aren't any other dependencies on it.

SqlException: Desktop --> Core isn't working because SqlErrorCollection is in the Desktop blob which is not serializable in Core. We ignore it during deserialization but it still throws a SerializationException indicating that the type isn't serializable. Making SqlErrorCollection serializable would also require to make SqlError serializable. Both types aren't doing anything special but SqlErrorCollection has a different data structure in Core than in Desktop (ArrayList vs List). That change would needed to be reverted.
ElementNotAvailableException & ElementNotEnabledException: They are both not exposed in the refs and would need a manual typeforward from UIAutomationTypes to Windows.UI.Xaml.Automation. I can't see it listed in src/shims/netfxreference.props, therefore I assume we don't have any typeforwards for it yet.
CryptoSignedXmlRecursionException is neither exposed in the ref nor in the src assembly. It would need to be made public in the src assembly and we would also need a manual typeforward from System.Security to System.Security.Cryptography.Xml. We don't already have one. I tried to make one but got some typeforward already existing error of a type. Not sure what's happening there.
XPathCompileException is internal in the impl. Same as CryptoSignedXmlRecursionException would needed to be made public and requires a manual typeforward from System.Data.SqlXml to System.Private.Xml. We don't have one yet.
XamlParseException & InvalidPrinterException: need to follow up on that. Can't reference it. Maybe need to conditionalize tests for certain target groups.
SecurityException has diverged a lot from Desktop as it is just a stub. It seems some information gets lost and a comparison is hardly possible. I need to follow up on that.

…x & MacOSX

ViktorHofer · 2017-10-26T15:15:41Z

@weshaggard I can't fix the last error happening here on my own and need your help. It seems to be related to the System.ComponentModel.DataAnnotations shim that gets created when building for uap. The shim can't be loaded, I guess something is faulted inside the testhost configuration.

Unable to find assembly 'System.ComponentModel.DataAnnotations, Version=4.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35'.

ViktorHofer · 2017-10-27T00:25:58Z

These are the remaining errors. Everything seems related to shims... @weshaggard

(uapaot) Unable to find assembly 'System.Configuration, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a':
- System.Configuration.ConfigurationErrorException
- System.Configuration.ConfigurationsErrorsException
- System.Configuration.Provider.ProviderException

(uapaot) Unable to find assembly 'System.Runtime.Serialization, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089':
- System.Runtime.Serialization.InvalidDataContractException

(uapaot) Unable to find assembly 'System.Transactions, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089':
- System.Transactions.TransactionAbortedException
- System.Transactions.TransactionException
- System.Transactions.TransactionInDoubtException
- System.Transactions.TransactionManagerCommunicationException
- System.Transactions.TransactionPromotionException

(uap & uapaot) Unable to find assembly 'System.ComponentModel.DataAnnotations, Version=4.0.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35':
- System.ComponentModel.DataAnnotations.ValidationException

(uapaot) Unable to find assembly 'System.Xml, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089':
- System.Xml.XPath.XPathException
- System.Xml.XmlException
- System.Xml.Schema.XmlSchemaException
- System.Xml.Schema.XmlSchemaInferenceException
- System.Xml.Schema.XmlSchemaValidationException
- System.Xml.Xsl.XsltCompileException
- System.Xml.Xsl.XsltException

…uap/uapaot

…oad errors

ViktorHofer · 2017-10-27T15:22:32Z

Tracking disabled uap/uapaot because of shim assembly loading issues here: https://github.com/dotnet/corefx/issues/24916

ViktorHofer · 2017-10-27T16:04:27Z

They nightly uapaot build will fail with an error indicating that an exception type isn't marked as serializable. This should be fixed when this change gets ingested with the next ProjectN build into corefx: dotnet/coreclr#14716

* Add serializable attribute and typeforward and adding serialization impl * Expose ZLibException in impl assembly * Remove deserialization negative tests * Adding tests for exceptions * Adding SqlError data to base exception data table * System Data Facade * Add netfx471 blob diffs for Hashtable and ListDictionary * Build Microsoft.NETCore.App.deps.json after manual shims * Disable currently failing uap/uapaot tests because of shim assembly load errors

…rds (#25047) * Make corefx exceptions serializable and add typeforwards (#24427) * Add serializable attribute and typeforward and adding serialization impl * Expose ZLibException in impl assembly * Remove deserialization negative tests * Adding tests for exceptions * Adding SqlError data to base exception data table * System Data Facade * Add netfx471 blob diffs for Hashtable and ListDictionary * Build Microsoft.NETCore.App.deps.json after manual shims * Disable currently failing uap/uapaot tests because of shim assembly load errors * Cleanup code in System.Runtime.Serialization.Formatters.Tests (#23940) * Cleanup code in System.Runtime.Serialization.Formatters.Tests * Moved UnitySerializationHolder serialization test to System.Runtime.Tests * Fix UpdateBlobs regression, reenabling ValueTuple tests on Core, add Dictionary<string,string> serialization test case (#24962) * Update non NetCoreApp package versions and build them

…fx#24427) * Add serializable attribute and typeforward and adding serialization impl * Expose ZLibException in impl assembly * Remove deserialization negative tests * Adding tests for exceptions * Adding SqlError data to base exception data table * System Data Facade * Add netfx471 blob diffs for Hashtable and ListDictionary * Build Microsoft.NETCore.App.deps.json after manual shims * Disable currently failing uap/uapaot tests because of shim assembly load errors Commit migrated from dotnet/corefx@e3f74de

ViktorHofer added the area-System.Runtime.Extensions label Oct 4, 2017

ViktorHofer self-assigned this Oct 4, 2017

ViktorHofer requested review from stephentoub, danmoseley and weshaggard October 4, 2017 16:41

dnfclas added the cla-already-signed label Oct 4, 2017

ViktorHofer force-pushed the SerializationExceptions branch 7 times, most recently from 1d9e797 to 01ee0cc Compare October 4, 2017 17:26

ViktorHofer changed the title ~~Add serializable attribute and typeforward and adding serialization impl~~ Make corefx exceptions serializable and add typeforwards Oct 4, 2017

ViktorHofer requested a review from morganbr October 4, 2017 20:51

weshaggard reviewed Oct 4, 2017

View reviewed changes

joperezr reviewed Oct 4, 2017

View reviewed changes

weshaggard reviewed Oct 4, 2017

View reviewed changes

ViktorHofer force-pushed the SerializationExceptions branch from 0ddb564 to 1afce60 Compare October 9, 2017 15:55

danmoseley reviewed Oct 9, 2017

View reviewed changes

ViktorHofer and others added 10 commits October 25, 2017 17:06

Move serialization members outside of netstandard1.3 configuration

5116af0

Update netfx blobs

f74cfed

Fix SocketException and NetworkInformationException test diffs on Uni…

0b0c94e

…x & MacOSX

Skip message for NIE

8888369

Skip message for SocketException

fb0ac7b

Attempt to fix NIE & SE

be9c7d6

Fix netfx471 blob diffs for Hashtable and ListDictionary

0bd0f3c

Disabling a few exception tests on uap

fd212ae

System Data Facade

31c2370

Build Microsoft.NETCore.App.deps.json after manual shims

2e0ab37

ViktorHofer force-pushed the SerializationExceptions branch from 7a6c867 to 2e0ab37 Compare October 25, 2017 15:06

Fix uap build build

21d8850

ViktorHofer added 2 commits October 27, 2017 03:32

Update rd.xml for uapaot build and disable reflection based tests on …

8de66ca

…uap/uapaot

Disable currently failing uap/uapaot tests because of shim assembly l…

dd322dc

…oad errors

ViktorHofer merged commit e3f74de into dotnet:master Oct 27, 2017

ViktorHofer deleted the SerializationExceptions branch October 27, 2017 16:01

karelz added this to the 2.1.0 milestone Oct 28, 2017

stukselbax mentioned this pull request Nov 15, 2017

Making Win32Exception.ToString() method contain NativeErrorCode #25216

Merged

YohDeadfall mentioned this pull request Mar 1, 2019

Cleaned up PostgresException npgsql/npgsql#2346

Merged

ViktorHofer mentioned this pull request Jan 31, 2020

Bring back exception serialization support dotnet/runtime#23732

Closed

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make corefx exceptions serializable and add typeforwards #24427

Make corefx exceptions serializable and add typeforwards #24427

ViktorHofer commented Oct 4, 2017 •

edited

Loading

ViktorHofer commented Oct 4, 2017 •

edited

Loading

weshaggard Oct 4, 2017

ViktorHofer Oct 5, 2017

joperezr Oct 4, 2017

ViktorHofer Oct 4, 2017

joperezr Oct 4, 2017

ViktorHofer Oct 4, 2017

joperezr Oct 5, 2017

ViktorHofer Oct 5, 2017

weshaggard Oct 4, 2017

ViktorHofer Oct 4, 2017

weshaggard commented Oct 4, 2017

danmoseley commented Oct 4, 2017

danmoseley commented Oct 4, 2017

ViktorHofer commented Oct 5, 2017 •

edited

Loading

ViktorHofer commented Oct 5, 2017

ViktorHofer commented Oct 9, 2017 •

edited

Loading

danmoseley Oct 9, 2017

danmoseley Oct 9, 2017

ViktorHofer commented Oct 13, 2017 •

edited

Loading

ViktorHofer commented Oct 26, 2017 •

edited

Loading

ViktorHofer commented Oct 27, 2017 •

edited

Loading

ViktorHofer commented Oct 27, 2017

ViktorHofer commented Oct 27, 2017

Make corefx exceptions serializable and add typeforwards #24427

Make corefx exceptions serializable and add typeforwards #24427

Conversation

ViktorHofer commented Oct 4, 2017 • edited Loading

ViktorHofer commented Oct 4, 2017 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weshaggard commented Oct 4, 2017

danmoseley commented Oct 4, 2017

danmoseley commented Oct 4, 2017

ViktorHofer commented Oct 5, 2017 • edited Loading

ViktorHofer commented Oct 5, 2017

ViktorHofer commented Oct 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ViktorHofer commented Oct 13, 2017 • edited Loading

ViktorHofer commented Oct 26, 2017 • edited Loading

ViktorHofer commented Oct 27, 2017 • edited Loading

ViktorHofer commented Oct 27, 2017

ViktorHofer commented Oct 27, 2017

ViktorHofer commented Oct 4, 2017 •

edited

Loading

ViktorHofer commented Oct 4, 2017 •

edited

Loading

ViktorHofer commented Oct 5, 2017 •

edited

Loading

ViktorHofer commented Oct 9, 2017 •

edited

Loading

ViktorHofer commented Oct 13, 2017 •

edited

Loading

ViktorHofer commented Oct 26, 2017 •

edited

Loading

ViktorHofer commented Oct 27, 2017 •

edited

Loading