Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BinaryFormatter persistent version conflict #109

Closed
CezarCretu opened this issue Nov 16, 2016 · 6 comments
Closed

BinaryFormatter persistent version conflict #109

CezarCretu opened this issue Nov 16, 2016 · 6 comments

Comments

@CezarCretu
Copy link

CezarCretu commented Nov 16, 2016

When using CacheManager with redis, the default serialization method is BinaryFormatter.
Binary formatter includes the versioned assembly name into its binary output (see here).

Due to this, if you cache some non-primitive value referenced in an assembly, and then change that assembly's version, CacheManager won't be able to solve version conflicts on its own.

From my understanding of the code, on update it pulls the old value and deserializes it, which works fine. Then the output gets serialized again for comparison with the value stored in cache, and this is where the problem arises.
Since the version is included in the BinaryFormatter output, the comparison will always fail since the binary array will never be the same.

@MichaCo
Copy link
Owner

MichaCo commented Dec 1, 2016

I will check if I can reproduce that somehow easily. Not sure if that's actually a real issue though.
And just use JSON instead of binary. Binary is pretty slow anyways.

@CezarCretu
Copy link
Author

Here's how you can reproduce this easily:

  • Create a project
  • Inside of that project, create a class
  • Set up CacheManager with redis
  • Cache an instance of the class created above
  • Change the assembly version
  • Try running an update on the key you just cached

I did just use JSON, I opened the issue to warn you that you ship with broken defaults.

I guess that if you can't find a reason to recommend BinaryFormatter serialization to anyone, you should consider changing the default to JSON - it's probably the least intrusive of the available options.

If nothing else, this issue could save some time for those that run into this problem, since what gets logged isn't very helpful in diagnosing it.

Also, moving the discussion away from serialization, I'm curios why this approach to optimistic concurrency was chosen. What benefits does ferrying the data around have vs just using some random token for versioning?

@MichaCo
Copy link
Owner

MichaCo commented Dec 1, 2016

@CezarCretu I meant, reproducing it via unit testing ;) The issue how to do it in general is clear.
Binary is the only thing supported by the full framework out of the box. To use json, you opt into another package. I don't want json references in the Core package.
That's the reason Binary is the default, and I think that's fine.
In case you use dotnet core, Binary doesn't exist anyways and you'll be forced to decide if you want JSON or protobuf or whatever...

Regarding your last point, I actually did an implementation with a versioning token, simple counter of some kind. That totally works, the concurrency model doesn't change though and it turned out, for small amount of data, the performance actually gets slightly worse.
But maybe it is more safe to use it, at least for binary, it would fix those issues you discovered.
So, instead of trying to fix that, I might change it back again to use a version counter.

Thanks anyways for testing all that stuff!
Btw, what exactly do you think should be logged so that the logs are more helpful?

@CezarCretu
Copy link
Author

CezarCretu commented Dec 1, 2016

Sure, programatically triggering ths is probably more effort than it's worth, seeing that you'd have to create a very specific test for it, and the issue will only reproduce under a very predictable set of circumstances.

Regarding logging, I don't think there's anything you can do, without looking for this exact issue. After all, it was telling me exactly what was going wrong, but my assumptions about what it was doing were preventing me from discovering the source of the problem.

You do mention that you don't want to reference a json serializer in the core packages, but also that the .net core version doesn't come with one at all. I think that nothing is as good of a default as anything, certainly better than using BinaryFormatter in this manner.

There are ways in which to fix BinaryFormatter for this exact issue, but it does seem like the best way to fix unexpected behavior is to not make the assumption that the serializer has a stable serialization -> deserialization -> serialization output.
After all, it's not exactly a BinaryFormatter specific issue. if I remember the inner workings of the library correctly, this could also be reproduced with a JSON serializer that encodes the keys with a non-deterministic ordering.

@MichaCo
Copy link
Owner

MichaCo commented Dec 2, 2016

There are ways in which to fix BinaryFormatter for this exact issue

Just FYI:
After some testing turns out that serializing and deserializing a byte array into a newer version of the assembly is not an issue. But even if I set FormatterAssemblyStyle.Simple, the serializer totally ignores that and the version bits are still present ==> the byte array is different anyways, which is bad.

@MichaCo MichaCo added this to the Version 0.9.3 milestone Dec 3, 2016
@MichaCo
Copy link
Owner

MichaCo commented Jan 11, 2017

@CezarCretu I've changed the update mechanism in the redis handle to use a version field, seems to work pretty good and should fix your issue. Will be in the next release

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants