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

serialize the access to mutable shared state in System.IO.Packaging #56367

Merged
merged 2 commits into from
Jul 27, 2021

Conversation

adamsitnik
Copy link
Member

PackageXmlStringTable.NameTable is static:

and it's being reused for every PartBasedPackageProperties instance:

XmlReaderSettings xrs = new XmlReaderSettings();
xrs.NameTable = _nameTable;
using (Stream stream = part.GetStream(FileMode.Open, FileAccess.Read))
// Create a reader that uses _nameTable so as to use the set of tag literals
// in effect as a set of atomic identifiers.

the problem is that it's mutable and can be modified by multiple threads at the same time (#43012)

We have few options:

  • make NameTable type thread-safe. Since it's not thread-safe by design (it's stated in docs) I am not sure if it's the best idea as it might be used properly in other libraries and all of them would pay the perf penalty
  • create a copy of the table for each XmlReaderSettings - there are no public methods that allow for that and the type is defined in different lib, moreover it seems that we do want to have global instance to use cheaper reference comparisions:
    // This System.Xml.NameTable makes sure that we use the same references to strings
    // throughout (including when parsing Xml) and so can perform reference comparisons
    // rather than value comparisons.
    private readonly NameTable _nameTable;
  • introduce locks in System.IO.Packaging. I think it's the lesser evil and this is what this PR does.

Fixes #43012

@ghost
Copy link

ghost commented Jul 27, 2021

Tagging subscribers to this area: @dotnet/area-system-io-compression
See info in area-owners.md if you want to be subscribed.

Issue Details

PackageXmlStringTable.NameTable is static:

and it's being reused for every PartBasedPackageProperties instance:

XmlReaderSettings xrs = new XmlReaderSettings();
xrs.NameTable = _nameTable;
using (Stream stream = part.GetStream(FileMode.Open, FileAccess.Read))
// Create a reader that uses _nameTable so as to use the set of tag literals
// in effect as a set of atomic identifiers.

the problem is that it's mutable and can be modified by multiple threads at the same time (#43012)

We have few options:

  • make NameTable type thread-safe. Since it's not thread-safe by design (it's stated in docs) I am not sure if it's the best idea as it might be used properly in other libraries and all of them would pay the perf penalty
  • create a copy of the table for each XmlReaderSettings - there are no public methods that allow for that and the type is defined in different lib, moreover it seems that we do want to have global instance to use cheaper reference comparisions:
    // This System.Xml.NameTable makes sure that we use the same references to strings
    // throughout (including when parsing Xml) and so can perform reference comparisons
    // rather than value comparisons.
    private readonly NameTable _nameTable;
  • introduce locks in System.IO.Packaging. I think it's the lesser evil and this is what this PR does.

Fixes #43012

Author: adamsitnik
Assignees: -
Labels:

area-System.IO.Compression

Milestone: 6.0.0

Copy link
Member

@stephentoub stephentoub left a comment

Choose a reason for hiding this comment

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

Any idea how frequently this is accessed? I'm a little concerned this will introduce a large bottleneck when used at scale. But I agree it's the simplest/safest solution.

@adamsitnik
Copy link
Member Author

Any idea how frequently this is accessed?

I am sorry, but no.

I'm a little concerned this will introduce a large bottleneck when used at scale

Most probably if this was used a lot at scale, we would be getting more bug reports related to this issue. If we ever receive a complain, we should reconsider making the base type thread-safe using some lock-free data structures like ConcurrentDictionary etc

@adamsitnik adamsitnik merged commit 72393ff into dotnet:main Jul 27, 2021
@adamsitnik adamsitnik deleted the threadSafeNameTable branch July 27, 2021 16:10
@ghost ghost locked as resolved and limited conversation to collaborators Aug 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Multi-threading bugs in System.IO.Packaging
2 participants