mbraceproject / FsPickler

A fast multi-format message serializer for .NET
http://mbraceproject.github.io/FsPickler/
MIT License
323 stars 54 forks source link

Picklers using Type.GetMembers #92

Closed haraldsteinlechner closed 6 years ago

haraldsteinlechner commented 7 years ago

For autogenerated picklers, FsPickler atm uses Reflection to retrieve the list of members. unfortunately Type.GetMember returns the members in arbitrary order (in general). This means that, pickling an object and unpickling the same object may fail due to different order of members (for application runs). In our case 2D structs get messed up their X and Y coordinates from time to time (nice bug to find ;))

The cause is here: https://msdn.microsoft.com/en-us/library/424c79hc.aspx -> "The GetMembers method does not return members in a particular order, such as alphabetical or declaration order. Your code must not depend on the order in which members are returned, because that order varies."

GetMembers is not pure. The cause can be found here: https://blogs.msdn.microsoft.com/haibo_luo/2006/07/10/member-order-returned-by-getfields-getmethods/

There are several non-solutions to this problem, e.g. https://stackoverflow.com/questions/9062235/get-properties-in-order-of-declaration-using-reflection

One pragmatic way would be to sort fields by name. Unfortunately this would break pickling of existing objects but i think this might be not that of a problem given the goals of fspickler (not being a persistent store library). What to you think? We would create a pull request using sorted members...

@krauthaufen @thomasortner @aszabo314

eiriktsarpalis commented 7 years ago

I agree. TBH I thought fields were already being sorted by name.

haraldsteinlechner commented 7 years ago

1) the CI fails for linux but works for windows though also windows reports errors regarding broken serializations? I still need some local testing which ATM does not work... EDIT: all green, just took quite some time ;)

2) i wonder if the same problem exists in TypeShape?

3) I think this one should be ok? https://github.com/mbraceproject/FsPickler/blob/master/src/FsPickler/PicklerGeneration/FSharpTypeGen.fs#L55

haraldsteinlechner commented 6 years ago

the bug appeared more and more often for my use cases, so at the moment i copy my version of fspickler to other projects. what is your stategy regrading this issue?

tebjan commented 6 years ago

This a serious issue and has produced quite some questions marks and headache on our side.

haraldsteinlechner commented 6 years ago

@eiriktsarpalis any news, thoughts on this?

eiriktsarpalis commented 6 years ago

@haraldsteinlechner I had left some feedback in your PR that fixes this

eiriktsarpalis commented 6 years ago

Yikes, I had forgotten to press the "submit review" button.

haraldsteinlechner commented 6 years ago

just fixed this one (still running CI)

eiriktsarpalis commented 6 years ago

Fixed in FsPIckler 4.0