Cross framework serialization finish up #20870

ViktorHofer · 2017-06-09T15:03:12Z

This PR is intended to be the last piece to make serialization work across Desktop and Core.

TODO

mscorlib shim as default entrance point for BinaryFormatter Using mscorlib shim as default resolving assembly for BinaryFormatter serialization instead of System.Private.Corelib #20879
CookieContainer data storage (Hashtable vs Dictionary)
Manual System.dll façade for TreeSet and CookieVariant Building manual System facade for serialization support #20988
Remaining manual equality checks in test code [@krwq]
OrdinalIgnoreCaseComparer does not exist on netfx Remove serializable attribute from OrdinalIgnoreCaseComparer coreclr#12215 & Fix GetHashCode for ordinal comparer coreclr#12267
Fix Uri.OriginalString and uncomment equality check [@krwq]
Fix Cookie.TimeStamp and uncomment equality check [@krwq]
Waiting for Fix GetHashCode for ordinal comparer coreclr#12267 to come into corefx

Status

Read from left to right. E.g.: Core deserializes to Core

	Core	Desktop
Core	x	x
Desktop	x	x

justinvp · 2017-06-09T15:17:07Z

CookieContainer data storage (Hashtable vs Dictionary)

Note that the nested CookieContainer.PathList type is stored in the CookieContainer's Hashtable/Dictionary... In the full framework, PathList uses the non-generic SortedList, but in CoreFX it uses the generic SortedList<T>.

ViktorHofer · 2017-06-09T15:19:50Z

Thanks Justin. I already know that. We will discuss how to handle CookieContainer and all involved types later today and will keep you updated here.

danmoseley

LGTM

ViktorHofer · 2017-06-09T19:32:51Z

@lmolkova Do you need Activity to be interchangeable between Core and Desktop or is it enough to be serializable from Core to Core?

lmolkova · 2017-06-09T19:58:55Z

I don't know how much context I miss.

I assume your question is related to cross AppDomain/process calls and presence of the Serializable attribute int the Activity. Note that Activity is serializable on NET 4.5 only.

The whole Activity is not intended to be passed outside of the process, only its certain properties should be propagated along with the request. E.g. if an application makes HTTP call, some properties of the current Activity are passed in HTTP headers. So some protocol should decide how Activity properties are serialized into a particular transport.

So Activity should not be interchangeable at all.

May be @vancem could also comment on that if I missed anything.

ViktorHofer · 2017-06-09T20:18:26Z

EDIT: the list was wrong :(

jkotas · 2017-06-09T20:22:04Z

custom equality check

What is it for?

ViktorHofer · 2017-06-09T20:22:22Z

@lmolkova Thanks for the clarification. That means we don't need to put a TypeForward attribute on it and test roundtripping it from netfx to core and back.

ViktorHofer · 2017-06-09T20:26:53Z

@jkotas to make sure that types are correctly serializing and deserializing from/to different platforms. Example given DataSet and DataTable don't override object Equals nor implement IEquatable. Therefore we need to manually check the cloned and original objects serializable fields for equality.

Some of these types are covered by xunit Assert.Equal (I think it also tests items in collections which implement IEnumerable/IList). But some of them are definitely not covered like specialized collections.

krwq · 2017-06-09T20:29:00Z

@ViktorHofer should we implement those in the product code?

jkotas · 2017-06-09T20:31:42Z

Example given DataSet and DataTable don't override object Equals nor implement IEquatable

Why is it a problem? When you serialize/deserialize object on full .NET Framework, Equals for master and clone returns true for some types and returns false for other types. That's by design. I do not think we need to be fixing it.

krwq · 2017-06-09T20:33:40Z

@jkotas - I believe this was meant for tests only so that we are able to asses if the type is cross-round-tripping (netfx->corefx) correctly

jkotas · 2017-06-09T20:33:42Z

I think I understand it now - it is just about making the test coverage better, not about fixing the product.

ViktorHofer · 2017-06-09T20:34:34Z

@ViktorHofer should we implement those in the product code?

If you want to go through API reviews for each type, then go for it. As this probably would take a lot of time and arguing and we want to test this till next week, I would not suggest it ;)

ViktorHofer · 2017-06-09T20:34:55Z

I think I understand it now - it is just about making the test coverage better, not about fixing the product.

Correct.

danmoseley · 2017-06-14T18:29:18Z

src/System.Net.Primitives/src/System/Net/CookieContainer.cs

@@ -810,22 +815,24 @@ private void BuildCookieCollectionFromDomainMatches(Uri uri, bool isSecure, int
                PathList pathList;
                lock (m_domainTable)


Oops, this should be lock (m_domainTable.SyncRoot)

DEfinitely should fix that.

Same issue on line 866 below the comment // (This is the only place that does domain removal)

danmoseley · 2017-06-14T18:30:39Z

src/System.Net.Primitives/src/System/Net/CookieContainer.cs

@@ -810,22 +815,24 @@ private void BuildCookieCollectionFromDomainMatches(Uri uri, bool isSecure, int
                PathList pathList;
                lock (m_domainTable)
                {
-                    if (!m_domainTable.TryGetValue(domainAttribute[i], out pathList))
+                    object pathListValue = m_domainTable[domainAttribute[i]];


There is no need for pathListValue you can just assign directly to pathList

Looks like desktop does that.

not changing

danmoseley · 2017-06-14T18:32:33Z

src/System.Net.Primitives/src/System/Net/CookieContainer.cs

@@ -950,7 +958,7 @@ internal string GetCookieHeader(Uri uri, out string optCookie2)

            string delimiter = string.Empty;

-            var builder = StringBuilderCache.Acquire();
+            StringBuilder builder = StringBuilderCache.Acquire();


FYI for ask-mode changes, we try to avoid unnecessary unrelated changes like this. Reasons are (1) distracting from actual diff (2) in general, adds risk. Not worth changing this now, especially because of the volume of other changes...

danmoseley · 2017-06-14T18:36:33Z

src/System.Net.Primitives/src/System/Net/CookieContainer.cs

                    count += cc.Count;
                }
            }
            return count;
        }

-        public CookieCollection this[string s]
+        public ICollection Values


do we need to add this?

Although I see desktop had it (and with no explicit lock) so I guess it is best to add it as you did.

danmoseley · 2017-06-14T18:37:34Z

src/System.Net.Primitives/src/System/Net/CookieContainer.cs


-        public static PathList Create() => new PathList(new SortedList<string, CookieCollection>(PathListComparer.StaticInstance));
+        public static PathList Create() => new PathList(SortedList.Synchronized(new SortedList(PathListComparer.StaticInstance)));


I'm not sure why this was .Synchronized before since evidently when we changed to the generic form we concluded that the locks below were enough. Especially since we exposed no way to write to it.

But, it is safest to match desktop here as you have done.

I'm not sure whether we decided to make synchronized SortedLists serializable. I'd suggest checking.

I know. That's why I changed it: https://github.com/dotnet/corefx/pull/20870/files#diff-f48fff7b7909f489c5e8b8599c22843cR614

davidsh · 2017-06-14T18:40:04Z

src/System.Net.Primitives/src/System/Net/Cookie.cs

@@ -48,15 +49,16 @@ public sealed class Cookie
        private CookieVariant m_cookieVariant = CookieVariant.Plain; // Do not rename (binary serialization)
        private bool m_discard = false; // Do not rename (binary serialization)
        private string m_domain = string.Empty; // Do not rename (binary serialization)
-        private bool m_domainImplicit = true; // Do not rename (binary serialization)
+        private bool m_domain_implicit = true; // Do not rename (binary serialization)


Why are these variables being renamed to use underscores?

Must match desktop to serialize between.

davidsh · 2017-06-14T18:40:41Z

src/System.Net.Primitives/src/System/Net/Cookie.cs


                    for (int i = 0; i < ports.Length; ++i)
                    {
                        // Skip spaces
                        if (ports[i] != string.Empty)
                        {
-                            if (!Int32.TryParse(ports[i], out port))
+                            if (!Int32.TryParse(ports[i], out int port))


what is the purpose of this change?

This looks like cleanup, which we can revert.

I reverted it

davidsh · 2017-06-14T18:41:37Z

src/System.Net.Primitives/src/System/Net/CookieCollection.cs

@@ -11,7 +11,7 @@ namespace System.Net
    //
    // A list of cookies maintained in Sorted order. Only one cookie with matching Name/Domain/Path
    [Serializable]
-    [System.Runtime.CompilerServices.TypeForwardedFrom("System, Version=4.0.0.0, PublicKeyToken=b77a5c561934e089")]
+    [System.Runtime.CompilerServices.TypeForwardedFrom("System, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089")]


Why are we adding "Culture=neutral" attributes? Is this different from .NET Framework?

Because we need it for BinaryFormatter blob optimization. If you want to know it in more detail just ping me

davidsh · 2017-06-14T18:42:33Z

src/System.Net.Primitives/src/System/Net/CookieContainer.cs

@@ -100,8 +100,7 @@ public class CookieContainer
            new HeaderVariantInfo(HttpKnownHeaderNames.SetCookie2, CookieVariant.Rfc2965)
        };

-        // NOTE: all accesses of _domainTable must be performed with _domainTable locked.
-        private readonly Dictionary<string, PathList> m_domainTable = new Dictionary<string, PathList>(); // Do not rename (binary serialization)
+        private readonly Hashtable m_domainTable = new Hashtable(); // Do not rename (binary serialization)


Why change to Hashtable?

Because Desktop uses Hashtable and we are using vanilla binary serialization so the types and names must match.

danmoseley · 2017-06-14T18:52:41Z

pathList field is defined as PathList which is a struct which doesn't support null checks

Ah yes I didn't see that. Fine.

justinvp · 2017-06-14T19:22:41Z

src/System.Net.Primitives/src/System/Net/CookieContainer.cs

            {
-                foreach (KeyValuePair<string, PathList> entry in m_domainTable)
+                foreach (DictionaryEntry entry in m_domainTable)


This is going to allocate a boxed object for each entry in the dictionary (see #7539, #9050, #10247). It'd be better to do something like:

// Manual use of IDictionaryEnumerator instead of foreach to avoid DictionaryEntry box allocations. IDictionaryEnumerator e = m_domainTable.GetEnumerator(); while (e.MoveNext()) { if (domain == null) { tempDomain = (string)e.Key; pathList = (PathList)e.Value; } ... }

We can change that later. Or even better, if you want to help you could spin up a PR which optimizes my recent changes without killing the just introduced serialization support.

justinvp · 2017-06-14T19:26:28Z

src/System.Net.Primitives/src/System/Net/CookieContainer.cs

                }

                lock (pathList.SyncRoot)
                {
-                    foreach (KeyValuePair<string, CookieCollection> pair in pathList)
+                    foreach (DictionaryEntry entry in pathList)


This is going to allocate a boxed object for each entry in the dictionary. I wonder if PathList.GetEnumerator should be changed to return IDictionaryEnumerator instead of IEnumerator, so the allocations can be avoided while iterating through pathList.

We can change that later. Or even better, if you want to help you could spin up a PR which optimizes my recent changes without killing the just introduced serialization support.

justinvp · 2017-06-14T19:26:48Z

src/System.Net.Primitives/src/System/Net/CookieContainer.cs

@@ -1061,14 +1072,15 @@ public object SyncRoot
        }

        [Serializable]
-        private sealed class PathListComparer : IComparer<string>
+        [System.Runtime.CompilerServices.TypeForwardedFrom("System, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089")]
+        sealed class PathListComparer : IComparer


I thought this was needed for cross framework serialization. I might be wrong

justinvp · 2017-06-14T19:31:36Z

src/System.Net.Primitives/src/System/Net/CookieContainer.cs

@@ -983,15 +991,16 @@ public void SetCookies(Uri uri, string cookieHeader)
    }

    [Serializable]
+    [System.Runtime.CompilerServices.TypeForwardedFrom("System, Version=4.0.0.0, Culture=neutral, PublicKeyToken=b77a5c561934e089")]
    internal struct PathList


Now that this is going back to being stored in Hashtable (and therefore is going to be boxed anyway), should it be changed back to a class?

We can change that later. Or even better, if you want to help you could spin up a PR which optimizes my recent changes without killing the just introduced serialization support.

morganbr · 2017-06-14T20:52:42Z

src/System.Net.Primitives/src/System/Net/CookieContainer.cs

@@ -737,8 +742,8 @@ internal CookieCollection InternalGetCookies(Uri uri)
            int port = uri.Port;
            CookieCollection cookies = null;

-            List<string> domainAttributeMatchAnyCookieVariant = new List<string>();
-            List<string> domainAttributeMatchOnlyCookieVariantPlain = null;
+            var domainAttributeMatchAnyCookieVariant = new Collections.Generic.List<string>();


Using a partial namespace here is a bit strange looking. Why not just leave it as List? If we need namespace qualification, please use the full namespace.

The compiler couldn't distinguish between SortedList generic and non-generic. I will add the fully namespace here.

morganbr · 2017-06-14T20:54:15Z

src/System.Net.Primitives/src/System/Net/CookieContainer.cs


-        public static PathList Create() => new PathList(new SortedList<string, CookieCollection>(PathListComparer.StaticInstance));
+        public static PathList Create() => new PathList(SortedList.Synchronized(new SortedList(PathListComparer.StaticInstance)));


I'm not sure whether we decided to make synchronized SortedLists serializable. I'd suggest checking.

morganbr · 2017-06-14T20:55:43Z

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

+            }
+            catch (Exception)
+            {
+                Console.WriteLine("Error during equality check of type " + obj?.GetType()?.FullName);


Can't you just add a message to the assert?

morganbr · 2017-06-14T21:02:33Z

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

+                BinaryFormatterTests.CheckEquals(@this.Comparer, other.Comparer)))
+                return false;
+
+            return BinaryFormatterTests.CheckSequenceEquals(@this, other);


I'm not sure ordering is guaranteed in different hash sets

@krwq please review this comment

@morganbr @ViktorHofer currently no failures related to that so seems like order is preserved

@krwq , I expect that ordering may change in the future, such as when #14354 is completed. It would be good if this test wasn't sensitive to that.

morganbr · 2017-06-14T21:03:09Z

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

+                @this.IsReadOnly == other.IsReadOnly &&
+                @this.IsFixedSize == other.IsFixedSize &&
+                @this.IsSynchronized == other.IsSynchronized &&
+                BinaryFormatterTests.CheckSequenceEquals(@this.Keys, other.Keys) &&


Same set ordering issue

morganbr · 2017-06-14T21:03:28Z

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

+                other != null &&
+                BinaryFormatterTests.CheckEquals(@this.Comparer, other.Comparer) &&
+                @this.Count == other.Count &&
+                BinaryFormatterTests.CheckSequenceEquals(@this.Keys, other.Keys) &&


Same set ordering issue

@krwq please review this comment

@morganbr @ViktorHofer because we don't have any failures related to that I'd leave as is - I wouldn't fix more than needed in this PR

morganbr · 2017-06-14T21:03:39Z

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

+        {
+            if (!(@this != null &&
+                other != null &&
+                BinaryFormatterTests.CheckSequenceEquals(@this.Keys, other.Keys) &&


Same set ordering issue

@krwq please review this comment

morganbr · 2017-06-14T21:03:53Z

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

+            if (!(@this != null &&
+                other != null &&
+                @this.Count == other.Count &&
+                BinaryFormatterTests.CheckSequenceEquals(@this.Keys, other.Keys) &&


Another possible set ordering issue

@krwq please review this comment

…er/corefx into SerilalizationFinishUp

danmoseley · 2017-06-14T21:38:44Z

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

+
+        public static bool IsEqual(this LinkedList<Point> @this, LinkedList<Point> other)
+        {
+            if (!(@this != null &&


@krwq many of these IsEqual implementations are missing this I thikn?

if (@this == null && other == null) return true;

Ah, that's handled in CheckSequenceEquals. fine.

I'm only fixing when I understand why the value is null, in many cases it should not be null even though that would be correct to check it in here

danmoseley · 2017-06-14T21:41:44Z

All LGTM as of now...

* wip: add stubs for custom isEqual * Updated blobs, small code refactoring, disabled CookieContainer test * Fixed netfx471 detection * add custom equals for graph with cycles * fix few more comparers * Merge * fix few more test cases * fix few more types, fix GetExtensionMethod to work with parameters with same type name but different generic params * finish tests (2 comparisons are commented out: search for TODO) * Adding TypeForward for DictionaryNode * Updating blobs * Update Cookie hashes, adding TypeForwards, removing unnecessary equality checks for CookieCollection * Reenabling SortedDictionary test * Update blobs * make cookie.Timestamp to roundtrip correctly * regenerate netfx and corefx blobs to account for Cookie.Timestamp change * make regex fix the blob for ObjectWithArray * fix ObjectWithArray netfx test failure * Replacing Dictionary with Hashtable for desktop serialization support * Update blobs * Fixing a lot of TypeForwards, adding another hashtable test, other fixes * Adding a few more TypeForwardedFrom attributes * Updating the coreclr version, disabling CookieContainer test * Update blobs * Disabling CompareInfo test --> waiting for coreclr * generate test cases * update blobs, comment out not working cases * fix null equality checks for new test cases and reenable them * PR feedback addressed * Fixed a typo... * Updating used coreclr version, enabling compareinfo test again

ViktorHofer added * NO MERGE * The PR is not ready for merge yet (see discussion for detailed reasons) area-Serialization labels Jun 9, 2017

ViktorHofer self-assigned this Jun 9, 2017

ViktorHofer requested review from danmoseley and morganbr June 9, 2017 15:03

dnfclas added the cla-already-signed label Jun 9, 2017

ViktorHofer force-pushed the SerilalizationFinishUp branch from 735a398 to bb170d7 Compare June 9, 2017 15:04

ViktorHofer changed the title ~~[WIP] Cross framework serialization finishing up~~ [WIP] Cross framework serialization finish up Jun 9, 2017

ViktorHofer force-pushed the SerilalizationFinishUp branch 3 times, most recently from a532ba1 to a0fc21a Compare June 9, 2017 17:53

danmoseley approved these changes Jun 9, 2017

View reviewed changes

morganbr approved these changes Jun 9, 2017

View reviewed changes

krwq force-pushed the SerilalizationFinishUp branch from 70556d9 to 5cd3774 Compare June 10, 2017 00:18

ViktorHofer force-pushed the SerilalizationFinishUp branch 2 times, most recently from bbc4e28 to 5b428ad Compare June 10, 2017 23:03

danmoseley reviewed Jun 14, 2017

View reviewed changes

davidsh reviewed Jun 14, 2017

View reviewed changes

justinvp reviewed Jun 14, 2017

View reviewed changes

krwq added 3 commits June 14, 2017 13:59

generate test cases

cf5f80a

update blobs, comment out not working cases

29641b0

fix null equality checks for new test cases and reenable them

b8d4fbb

morganbr approved these changes Jun 14, 2017

View reviewed changes

ViktorHofer added 3 commits June 14, 2017 23:22

PR feedback addressed

2cf5328

Merge branch 'SerilalizationFinishUp' of https://github.com/ViktorHof…

18d2494

…er/corefx into SerilalizationFinishUp

Fixed a typo...

19bcf06

danmoseley reviewed Jun 14, 2017

View reviewed changes

Updating used coreclr version, enabling compareinfo test again

6718144

ViktorHofer merged commit cdfb6ec into dotnet:master Jun 14, 2017

justinvp mentioned this pull request Jun 15, 2017

CookieContainer improvements #21074

Closed

ViktorHofer deleted the SerilalizationFinishUp branch September 13, 2018 20:24

		@@ -810,22 +815,24 @@ private void BuildCookieCollectionFromDomainMatches(Uri uri, bool isSecure, int
		PathList pathList;
		lock (m_domainTable)


		public static PathList Create() => new PathList(new SortedList<string, CookieCollection>(PathListComparer.StaticInstance));
		public static PathList Create() => new PathList(SortedList.Synchronized(new SortedList(PathListComparer.StaticInstance)));

Cross framework serialization finish up #20870

Cross framework serialization finish up #20870

Conversation

ViktorHofer commented Jun 9, 2017 • edited by danmoseley Loading

TODO

Status

justinvp commented Jun 9, 2017

ViktorHofer commented Jun 9, 2017

danmoseley left a comment

Choose a reason for hiding this comment

ViktorHofer commented Jun 9, 2017 • edited Loading

lmolkova commented Jun 9, 2017 • edited Loading

ViktorHofer commented Jun 9, 2017 • edited Loading

jkotas commented Jun 9, 2017

ViktorHofer commented Jun 9, 2017 • edited Loading

ViktorHofer commented Jun 9, 2017 • edited Loading

krwq commented Jun 9, 2017

jkotas commented Jun 9, 2017 • edited Loading

krwq commented Jun 9, 2017

jkotas commented Jun 9, 2017

ViktorHofer commented Jun 9, 2017

ViktorHofer commented Jun 9, 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

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

danmoseley Jun 14, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Jun 14, 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

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

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

Choose a reason for hiding this comment

danmoseley commented Jun 14, 2017

ViktorHofer commented Jun 9, 2017 •

edited by danmoseley

Loading

ViktorHofer commented Jun 9, 2017 •

edited

Loading

lmolkova commented Jun 9, 2017 •

edited

Loading

ViktorHofer commented Jun 9, 2017 •

edited

Loading

ViktorHofer commented Jun 9, 2017 •

edited

Loading

ViktorHofer commented Jun 9, 2017 •

edited

Loading

jkotas commented Jun 9, 2017 •

edited

Loading

danmoseley Jun 14, 2017 •

edited

Loading