Add a version header to Object-Streams. #15794
Conversation
…t if the persistence version has changed, and if we can read the data properly or not.
|
Tagging @jasonmalinowski @tmat @heejaechang @mattwar This supersedes: #15787 Here we actually store a two-byte version at teh start of the object-stream. That way the reader can tell up front if the persistence format has changed, and can immediately return null. In cases where we we are only serializing and not persisting across sessions, we assert that this should always succeed. In cases where we persist across sessions, we are now resilient to getting null back. I've also narrowed our exception checking to only IOException. @heejaechang Does your esent storage layer ever throw any sort of esent specific exceptions? Or does is swallow/translate them? |
|
esent doesn't leak out any exception. it catch all exception and return false. |
| /// this version, just change VersionByte2. | ||
| /// </summary> | ||
| internal const byte VersionByte1 = 0b10101010; | ||
| internal const byte VersionByte2 = 0b00000001; |
| return null; | ||
| } | ||
|
|
||
| return new StreamObjectReader(stream, knownObjects, binder, cancellationToken); |
There was a problem hiding this comment.
You could put the read of the format (recursive, etc) byte here too and have it also return null if it doesn't match one of the known types.
There was a problem hiding this comment.
I could. But that indicates a true bug. That means the format changed and the version wasn't updated. That's actually a bug, and i want an exceptoin in that case.
| { | ||
| Debug.Assert(reader != null, | ||
| @"We only ge a reader for data transmitted between live processes. |
|
👍 |
|
test this please |
Great. That simplifies things enormously. |
|
@MattGertz For approval. Yesterday i checked in a mitigation for #15780. But this is an actual fix. Filling out template now. |
This way we can tell, up front if the persistence version has changed, and if we can read the data properly or not.
Fixes #15780
Customer scenario
Customer upgrades from previous version of VS (including Dev14 or an RC build) to RTM. Customer opens an existing project in Dev15. VS crashes. Problem persists until they delete their .vs folder (which they have no idea is the cause).
Bugs this fixes:
Fixes #15780
Workarounds, if any
Customer deletes their .vs folder for every project they're opening in Dev15. This is pretty bad as there is no way for them to know to do this.
Risk
Extremely low. This is just adding a check that our on-disk format is what we expect. If it isn't we know the previous cache isn't safe to use and we build a new one.
Performance impact
Low/None. We're just checking an additional 2-bytes to validate that the disk format is one we expect.
Is this a regression from a previous update?
Yes.
Root cause analysis:
A recent PR changed our on-disk format for persisting data (like Find-References indices). This chnage happened without any augmentation to the format indicating what version it was. So the new code had no way to say "this persisted stream of bytes has the wrong format, we can't read this", and would instead crash.
How was the bug found?
This hit most members of teh roslyn team immediately.
To help avoid this, we're investing more in adding actual unit tests to test upgrade scenarios.