Replace BinaryFormatter with MessagePack serialization #1166
Conversation
There was a problem hiding this comment.
I was looking at the dependencies, and the MessagePack library only seems to be compatible with .net standard 2.0.
https://www.nuget.org/packages/MessagePack/2.5.140#dependencies-body-tab
Should we start removing the targeting for .net standard 2.1?
There was a problem hiding this comment.
oops. meant to put this comment on the Microsoft.Spark.csproj.
There was a problem hiding this comment.
not sure why the netstandard2.1 tf was there before, looks redundant to me
|
Can someone please help with this merge? Or assign to reviewers who have write access? This introduces dependencies on :
@arsdragonfly Have you already found someone to help you merge this, by any chance? |
|
@luisquintanilla @GoEddie |
Sorry I don’t |
|
@arsdragonfly => Great job! You are one of the pioneering advocates. Could you help identify who has the right to trigger the merge? I will write you en email. |
|
With this merge, what version of Spark will this support? Spark 3.3 or beyond to spark 3.5.1? |
Yes, but I think we need to know what cloud platforms we want to support. I think there are some minor tweaks needed for HDInsight. I'm guessing we should limit ourselves to HDInsight for now (and local OSS on windows). Then we need to decide if we use a new version number or stick with 2.1.x. The amount of work to be done is non-trivial, maybe about 40 hours to get up and running with .net 8 on HDI. Most of the work is non-technical and NOT an area where I add value (ie. management and decision making and communicating). I don't want to host this in my own fork forever. Have you tried to contact Microsoft? Do you have a .net foundation membership? Frankly, I'd prefer if you owned a fork, and I'll send you PR's to merge. Your help would be invaluable, whether technical or non-technical. I can send you PR's right away for .net 8 and messagepack and HDI compatibility. |
|
Start small..we recruit more to join. Local Windows 11 OS is key. Ideally we show the spark dotnet book examples work in e.g. Net So new and existing users can read the book and yet doing exercises code based on |
|
Updated |
The versions of Spark are probably the same as they were in Microsoft.Spark 2.1.1. I am only testing Spark 3.1.2. (Version supported for HDI 5) That has been sitting around for a long time. I am pretty sure we could get to other versions of spark if needed. It seems like the least of our concerns at this point. What is the concern insofar as Spark versioning? Please remember that .net and spark are pretty independent of each other. You can support a wide range of versions without too much difficulty. |
|
Just start very small..others will join with renewed interest |
|
still discussing internally on how to structure project governance, stay tuned |
|
Note to @AFFogarty and other MSFT reviewers: MessagePack's "typeless" functionality is banned for MSFT internal use under the same regulation that bans BinaryFormatter for MSFT internal use. If you accept this PR, you will still have to jump through the same "ask upper management for an exception to use this" hoops that you would've had to go through with BF. As OP notes, you could make an argument that since no logical trust boundary is being crossed, it's all a moot point anyway and the exception should be granted. I imagine your security team would sign off on that statement. But the main point I'm trying to make is that this PR does not simplify any internal signoff + release process. This PR should be viewed wholly through the lens of broader ecosystem impact, nothing more. I am only posting this comment because I am the compliance owner for serialization related technologies and I wanted to clarify how this PR would / would not impact any signoff. |
Thanks for the reminder. This is the unfortunate workaround that we'd probably have to settle around since BF is on its way out and the strictly statically typed alternative that Orleans folks chose to use since Orleans 7 will IMO make .NET for Spark completely useless for anything but the most trivial toy programs. @luisquintanilla could you help drive the security review/signoff process for this particular PR and for our future community-driven development model in general? |
| @@ -20,7 +20,6 @@ namespace Microsoft.Spark | |||
| /// also attempts to distribute broadcast variables using efficient broadcast algorithms to | |||
| /// reduce communication cost. | |||
| /// </summary> | |||
| [Serializable] | |||
| public sealed class Broadcast<T> : IJvmObjectReferenceProvider | |||
| { | |||
| [NonSerialized] | |||
There was a problem hiding this comment.
Let's replace [NonSerialized] wtih [IgnoreMember] attribute from MessagePack to ensure non-breaking changes
| @@ -20,7 +20,6 @@ namespace Microsoft.Spark | |||
| /// also attempts to distribute broadcast variables using efficient broadcast algorithms to | |||
| /// reduce communication cost. | |||
| /// </summary> | |||
| [Serializable] | |||
| public sealed class Broadcast<T> : IJvmObjectReferenceProvider | |||
There was a problem hiding this comment.
In order to register broadcast var in the registry on it being serialized, we can implement IMessagePackSerializationCallbackReceiver, and rename existing OnSerialized to OnBeforeSerialize(). Otherwise it's impossible to access var from worker context
| // TODO: find out why MessagePack is requiring a parameterless constructor. | ||
| public TestClass() | ||
| { | ||
| } |
There was a problem hiding this comment.
There are several more classes that require similar empty ctors, such as ExternalClass or Broadcast
There was a problem hiding this comment.
Now that the standalone nuget package for BF is now available, I'm leaning towards scraping the whole PR and moving to that instead, because this is IMO a major limitation that could break any code that uses user/3rd party classes that reference a class without an empty ctor. What do you think?
There was a problem hiding this comment.
Since microsoft doesn't add [Serializable] attributes to new types since .NET 6, sticking to NuGet will still not work long-term - As soon as there's a new commonly used type - integration will fall apart.
Imho the best option long-term would be either to investigate MsgPack further (Custom resolver for classes without default ctor and workaround for the bug with empty stream), or, if there's no way to work-around ctor behavior - make serializer configurable and include both options
I created a PR with version bump up for .NET and dependencies, for .NET8 it's still possible to use BinaryFormatter from library, for .NET9 we'll have to migrate to NuGet
| public object Deserialize(Stream stream, int length) | ||
| { | ||
| return _formater.Deserialize(stream); | ||
| return MessagePackSerializer.Typeless.Deserialize(stream); | ||
| } |
There was a problem hiding this comment.
There's a bug in MessagePack I found while tried to reuse these changes, when called from Collector::Collect: Deserializer erases entire stream after the first read.
Workaround for it is to either read stream in small chunks and pass to MessagePack, or load entire stream in MemoryStream and then use it.
|
do not merge this PR yet (see above mentioned problems) |
Fixes #1131 . As explained in my comment #1131 the whole thing is very much pointless due to the nature of Spark being RCE so we must be able to pass arbitrary data/computation around (except for one place where we don't really need the flexibility of deserializing arbitrary types and I've replaced that with a strongly typed deserializer). Nevertheless, using MessagePack's typeless serialization will hopefully saves us the trouble of the ecosystem downstream.