Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Remove syncroot fields from collections #34198

Merged
merged 12 commits into from
Dec 22, 2018
Merged

Remove syncroot fields from collections #34198

merged 12 commits into from
Dec 22, 2018

Conversation

@danmoseley
Copy link
Member

@danmoseley danmoseley commented Dec 21, 2018

CoreFX part of https://github.com/dotnet/corefx/issues/34149.
This is everything save (1) immutable collections, which I don't want to mess with (2) some marginal types and legacy libraries.

Also fix a typo in build-native.cmd.

@danmoseley
Copy link
Member Author

danmoseley commented Dec 21, 2018

Some tests to fix.

@danmoseley
Copy link
Member Author

danmoseley commented Dec 21, 2018

OrderedDictionary and LinkedList neglected to mark their _syncRoot with [NonSerializable]. They implement ISerializable, so that shouldn't affect regular binary serialization, but perhaps data contract serialization?

@danmoseley
Copy link
Member Author

danmoseley commented Dec 21, 2018 •
edited
Loading

Hmm, it did break one serialization test, BinaryFormatterTests.ValidateAgainstBlobs(obj: CookieContainer. The diff is roughly that System.Object\0\0\0\0\x10\f\0\0\0\0\0\0\0\x04\0\0\0\x1b is lost and it is fairly clear that is the object in the _syncRoot of a non generic SortedList because CookieContainer contains a PathList which contains some SortedLists including one that is a SyncSortedList. And SyncSortedList accidentally did not put [NonSerialized] on its _root field which it copies the _syncRoot into. And we effectively changed the type of _syncRoot from object to SortedList. Which may not be actually breaking, since SyncSortedList._root is typed as object, but it's not worth bothering with.

I will revert SortedList<T> .. fortunately it's not widely used.

ViktorHofer
ViktorHofer reviewed Dec 21, 2018
View reviewed changes
@karelz karelz added this to the 3.0 milestone Dec 21, 2018
@danmoseley danmoseley requested a review from jkotas December 21, 2018 19:19
jkotas
jkotas reviewed Dec 21, 2018
View reviewed changes
src/System.Collections.NonGeneric/src/System/Collections/SortedList.cs
private ValueList valueList; // Do not rename (binary serialization)
[NonSerialized]
private object _syncRoot;
private object _syncRoot; // Serialized indirectly in SyncSortedList
Copy link
Member

@jkotas jkotas Dec 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is the indirect serialization a problem?

Copy link
Member Author

@danmoseley danmoseley Dec 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's not. It does however mean that if you change the type of it, the binary format of SyncSortedList changes, causing our tests to break. I am not sure whether that would break cross version serialization. See note above.

Copy link
Member Author

@danmoseley danmoseley Dec 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And of course if you stored something in there that wasn't binary serializable, it would be a break.

Copy link
Member

@jkotas jkotas Dec 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure whether that would break cross version serialization.

I do not think it would break it.

Copy link
Member Author

@danmoseley danmoseley Dec 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, but it would if I changed SortedList to put something unserializable in here, so a comment seemed useful.

Copy link
Member

@jkotas jkotas Dec 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do not understand ... if you delete this field and just return this from SyncRoot, everything should still work fine.

Yes, the blob will be different but that should be ok.

Copy link
Member Author

@danmoseley danmoseley Dec 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright will do

Copy link
Member Author

@danmoseley danmoseley Dec 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mind if I do that in a follow up? Updating the binary blobs causes many other updates that I don't understand yet. I need to talk to @ViktorHofer

Copy link
Member

@jkotas jkotas Dec 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

@jkotas
Copy link
Member

jkotas commented Dec 21, 2018

They implement ISerializable

When you implement ISerializable, the [NonSerialized] attributes are irelevant. That's the contract. E.g. Dictionary does not have the _syncRoot marked [NonSerialized] either.

@danmoseley
Copy link
Member Author

danmoseley commented Dec 21, 2018

When you implement ISerializable, the [NonSerialized] attributes are irelevant. That's the contract. E.g. Dictionary does not have the _syncRoot marked [NonSerialized] either.

Yes, but I am not sure that is true for Data Contract Serialization.

@danmoseley
Copy link
Member Author

danmoseley commented Dec 21, 2018

Any more feedback?

jkotas
jkotas approved these changes Dec 21, 2018
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@danmoseley danmoseley merged commit 1fd3717 into dotnet:master Dec 22, 2018
@danmoseley danmoseley deleted the syncroot branch December 22, 2018 00:22
@Wraith2 Wraith2 mentioned this pull request Jan 11, 2019
Merged
josalem pushed a commit to josalem/corefx that referenced this pull request Jan 24, 2019
@danmoseley
Remove syncroot fields from collections (dotnet#34198)
8994dbf 
* Syncroot

* Fix typo in build-native

* Specialized

* Remove noise

* Revert non generic SortedList

* Test fixups

* CLR test fixes

* Revert "CLR test fixes"

This reverts commit 8db135c.

* Disable syncroot tests for NETFX

* Update QueueTests.cs

* Test fixes

* Typo

(cherry picked from commit 1fd3717)

Porting above changes from master sans changes to Build.cmd and Build-Native.cmd
@josalem josalem mentioned this pull request Jan 25, 2019
Closed
@filipnavara filipnavara mentioned this pull request Jan 28, 2019
Merged
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
@danmoseley
Remove syncroot fields from collections (dotnet/corefx#34198)
439637f 
* Syncroot

* Fix typo in build-native

* Specialized

* Remove noise

* Revert non generic SortedList

* Test fixups

* CLR test fixes

* Revert "CLR test fixes"

This reverts commit dotnet/corefx@8db135c.

* Disable syncroot tests for NETFX

* Update QueueTests.cs

* Test fixes

* Typo


Commit migrated from dotnet/corefx@1fd3717
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers

@ViktorHofer ViktorHofer ViktorHofer left review comments

@jkotas jkotas jkotas approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.0

Development

Successfully merging this pull request may close these issues.

4 participants

@danmoseley @jkotas @ViktorHofer @karelz