Remove BinaryFormatter from StateFileBase by Forgind · Pull Request #6350 · dotnet/msbuild

Forgind · 2021-04-15T17:58:21Z

This ends our reliance on BinaryFormatter within any StateFileBase. This comprises AssemblyReferenceCache, ResolveComReference, and ResGenDependencies. SystemState (part of ResolveAssemblyReference) also extends StateFileBase, but that was already converted. We may want to combine those two serialization efforts into a single location, but that's more of a refactor so lower priority.

Also added tests.

src/Tasks/ResGenDependencies.cs

src/Tasks/TaskTranslatorHelpers.cs

rokonec · 2021-04-16T14:31:24Z

src/Tasks/StateFileBase.cs

-                        if (retVal == null && deserializedObject != null)
+                        // If retVal is still null or the version is wrong, log a message not a warning. This could be a valid cache with the wrong version preventing correct deserialization.
+                        // For the latter case, internals may be unexpectedly null.
+                        if (retVal == null || retVal._serializedVersion != CurrentSerializationVersion)


Since _serializedVersion is never serialized, it will always have same value as CurrentSerializationVersion and it will never detect outdated cache.
In SystemState.DeserializeCacheByTranslator I have implemented some kind of file signature check along with basic versioning.

Good point, added.

src/Tasks/TaskTranslatorHelpers.cs

src/Tasks.UnitTests/AssemblyDependency/ResolveAssemblyReferenceCacheSerialization.cs

rainersigwald · 2021-04-20T15:07:08Z

src/Tasks/TaskTranslatorHelpers.cs

+                dict = new Dictionary<string, DateTime>(comparer);
+                translator.Translate(ref count);
+                string key = string.Empty;
+                DateTime val = DateTime.Now;


Would it be harmful to default this to min or max? Those are constants and would never require a clock read.

Suggested change

DateTime val = DateTime.Now;

DateTime val = DateTime.MinValue;

Nope, just needed a value.

rainersigwald · 2021-04-20T15:07:58Z

src/Tasks/TaskTranslatorHelpers.cs

            }
        }
+
+        public static void TranslateDictionary(this ITranslator translator, ref Dictionary<string, DateTime> dict, StringComparer comparer)


Why is this overload not in BinaryTranslator.cs with the rest? It doesn't use a Tasks-specific type.

To do this, I would have to add a method signature in ITranslator.cs and a separate implementation for read and write. Those implementations would be almost identical and easily merged. Looking at other examples in that class, most just reimplemented the same logic twice with a tiny tweak. I don't approve of wasting code like that and would propose moving all methods beyond the basics (int, string, etc.) to an extensions class where they can reuse code properly. I'm fine with moving this to Shared, though.

src/Tasks/SystemState.cs

rainersigwald · 2021-04-20T15:15:04Z

src/Tasks/SystemState.cs

-        /// <summary>
-        /// Cached delegate.
-        /// </summary>
-        private FileExists fileExists;


What's the motivation for this change?

It isn't used, and I came across it. Small cleanup.

rainersigwald · 2021-04-20T15:24:59Z

src/Tasks/Dependencies.cs

+        {
+            translator.TranslateDictionary(ref dependencies, (ITranslator translator, ref DependencyFile dependency) =>
+            {
+                if (t == typeof(ResGenDependencies.ResXFile))


Why pass and check the type explicitly? Could you instead do a pattern match on the runtime type?

Great question—that's actually how I first implemented it, and it took me a bit to figure out why it was failing. For serializing to disk, yes, that works. For deserializing, dependency comes in as null, so it can be cast to either a ResXFile or a PortableLibraryFile. Only one of those is valid, however, so I need to pick the right one. Explicitly passing the type (or some equivalent information) is the only way I can get the requisite information for deserializing the right type.

Can we make Dependencies implement ITranslatable and override it in derived classes?

If it was the first instinct for both of us it deserves a comment please

@rokonec The type is just Dictionary<string, DependencyFile> and it has to work for either Dictionary<string, ResXFile> or Dictionary<string, PortableLibraryFile>. The Dependencies class doesn't know which will be required, so the Translator can't either. To be honest, this whole class structure could have been dramatically simplified by just having ResGenDependencies have a Dictionary<string, ResXFile> and a Dictionary<string, PortableLibraryFile>. That would make this a lot cleaner but be a little outside the scope of this PR. That said, if you want me to do that, I'm happy deleting a lot of code 😉

rainersigwald · 2021-04-20T15:27:23Z

src/Tasks/ResGenDependencies.cs

            // First, try to retrieve the resx information from our hashtable.
-            var retVal = (ResXFile)resXFiles.GetDependencyFile(resxFile);
-            if (retVal == null)
+            if (resXFiles.GetDependencyFile(resxFile) is not ResXFile retVal || retVal == null)


I don't feel like this is clearer than switching the cast to an as.

That's probably better, thanks. This wasn't about clarity but correctness—the cast was failing and throwing an exception because it wasn't a ResXFile. That shouldn't happen in practice, and it's now fixed (see previous comment), but did cause problems for a bit.

rainersigwald · 2021-04-27T15:10:29Z

src/Shared/BinaryTranslator.cs

            }

+            /// <summary>
+            /// Translates a dictionary of { string, T } for dictionaries with public parameterless constructors.


Suggested change

/// Translates a dictionary of { string, T } for dictionaries with public parameterless constructors.

/// Translates a dictionary of { string, DateTime }.

rainersigwald · 2021-04-27T15:11:12Z

src/Shared/BinaryTranslator.cs

+            {
+                int count = 0;
+                dictionary = new(comparer);
+                Translate(ref count);


Can you use _reader.ReadInt32() here like other implementations?

rainersigwald · 2021-04-27T15:13:15Z

src/Tasks.UnitTests/AssemblyDependency/ResolveAssemblyReferenceCacheSerialization.cs

-        }
-
-        [Fact]
-        public void WrongFileVersion()


So is there now no equivalent to this test? What about the "cache was written with v.last and is read with v.current" scenario?

I can reinstate it. To be honest, in retrospect, I'm not sure why I deleted it in the first place.

src/Shared/BinaryTranslator.cs

This reverts commit 20d31f0.

Forgind added 2 commits April 15, 2021 10:15

Serialize state files without BinaryFormatter

8124add

Add tests

013f65d

rainersigwald reviewed Apr 15, 2021

View reviewed changes

src/Tasks/ResGenDependencies.cs Outdated Show resolved Hide resolved

src/Tasks/TaskTranslatorHelpers.cs Outdated Show resolved Hide resolved

marcpopMSFT linked an issue Apr 15, 2021 that may be closed by this pull request

BinaryFormatter Deprecation #6215

Closed

27 tasks

Remove extra [Serializable]

708b7d9

rokonec requested changes Apr 16, 2021

View reviewed changes

Forgind added 2 commits April 16, 2021 14:34

Shift serialization to StateFileBase and extensions

fdc980a

Add copyright header and remove fileExists

fc25eb7

rainersigwald reviewed Apr 20, 2021

View reviewed changes

Forgind added 2 commits April 20, 2021 10:45

Partially implement suggestions

76b9923

Remove Dependencies

858a99b

rainersigwald approved these changes Apr 27, 2021

View reviewed changes

Comments. Also centralize version

69ad8b7

danmoseley reviewed Apr 28, 2021

View reviewed changes

src/Shared/BinaryTranslator.cs Show resolved Hide resolved

rokonec approved these changes Apr 28, 2021

View reviewed changes

Forgind mentioned this pull request Apr 30, 2021

Rar permissions fix #6393

Merged

Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Apr 30, 2021

Forgind merged commit 20d31f0 into dotnet:main May 24, 2021

Forgind deleted the next-bf-removal branch May 24, 2021 21:50

Forgind added a commit to Forgind/msbuild that referenced this pull request May 26, 2021

Revert "Remove BinaryFormatter from StateFileBase (dotnet#6350)"

2bf9ea7

This reverts commit 20d31f0.

Forgind mentioned this pull request Jun 21, 2021

Update stage 0 SDK dotnet/installer#10940

Merged

Forgind mentioned this pull request Jul 9, 2021

RAR Cache should be versioned (TranslateDictionary) #6659

Closed

baronfel mentioned this pull request Jun 1, 2023

BinaryFormatter Deprecation #6215

Closed

27 tasks

	DateTime val = DateTime.Now;
	DateTime val = DateTime.MinValue;

	/// Translates a dictionary of { string, T } for dictionaries with public parameterless constructors.
	/// Translates a dictionary of { string, DateTime }.

Conversation

Forgind commented Apr 15, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Forgind Apr 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Forgind Apr 20, 2021 •

edited

Loading