diff --git a/src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.cs b/src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.cs index 0ab52cc061241..820818d55b601 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Resources/ResourceReader.cs @@ -27,26 +27,16 @@ namespace System.Resources // default file format. // - internal struct ResourceLocator + internal readonly struct ResourceLocator { - internal object? _value; // Can be null. - internal int _dataPos; - internal ResourceLocator(int dataPos, object? value) { - _dataPos = dataPos; - _value = value; + DataPosition = dataPos; + Value = value; } - internal int DataPosition => _dataPos; - - // Allows adding in profiling data in a future version, or a special - // resource profiling build. We could also use WeakReference. - internal object? Value - { - get => _value; - set => _value = value; - } + internal int DataPosition { get; } + internal object? Value { get; } internal static bool CanCache(ResourceTypeCode value) { diff --git a/src/libraries/System.Private.CoreLib/src/System/Resources/RuntimeResourceSet.cs b/src/libraries/System.Private.CoreLib/src/System/Resources/RuntimeResourceSet.cs index f6de75e7e3e98..e3cd57d74cf76 100644 --- a/src/libraries/System.Private.CoreLib/src/System/Resources/RuntimeResourceSet.cs +++ b/src/libraries/System.Private.CoreLib/src/System/Resources/RuntimeResourceSet.cs @@ -1,18 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. -/*============================================================ -** -** -** -** -** -** Purpose: CultureInfo-specific collection of resources. -** -** -===========================================================*/ - -#nullable enable using System.Collections; using System.Collections.Generic; using System.Diagnostics; @@ -178,7 +166,7 @@ sealed class RuntimeResourceSet : ResourceSet, IEnumerable private Dictionary? _resCache; - // For our special load-on-demand reader, cache the cast. The + // For our special load-on-demand reader. The // RuntimeResourceSet's implementation knows how to treat this reader specially. private ResourceReader? _defaultReader; @@ -189,32 +177,19 @@ sealed class RuntimeResourceSet : ResourceSet, IEnumerable // don't exist. private Dictionary? _caseInsensitiveTable; - // If we're not using our custom reader, then enumerate through all - // the resources once, adding them into the table. - private bool _haveReadFromReader; - #if !RESOURCES_EXTENSIONS - internal RuntimeResourceSet(string fileName) : base(false) + internal RuntimeResourceSet(string fileName) : + this(new FileStream(fileName, FileMode.Open, FileAccess.Read, FileShare.Read)) { - _resCache = new Dictionary(FastResourceComparer.Default); - Stream stream = new FileStream(fileName, FileMode.Open, FileAccess.Read, FileShare.Read); - _defaultReader = new ResourceReader(stream, _resCache, false); - Reader = _defaultReader; } - internal RuntimeResourceSet(Stream stream, bool permitDeserialization = false) : base(false) + internal RuntimeResourceSet(Stream stream, bool permitDeserialization = false) : + base(false) { _resCache = new Dictionary(FastResourceComparer.Default); _defaultReader = new ResourceReader(stream, _resCache, permitDeserialization); - Reader = _defaultReader; } #else - private -#if NETFRAMEWORK - new -#endif - IResourceReader Reader => _defaultReader!; - internal RuntimeResourceSet(IResourceReader reader) : // explicitly do not call IResourceReader constructor since it caches all resources // the purpose of RuntimeResourceSet is to lazily load and cache. @@ -235,32 +210,18 @@ internal RuntimeResourceSet(IResourceReader reader) : protected override void Dispose(bool disposing) { - if (Reader == null) + if (_defaultReader is null) return; if (disposing) { - lock (Reader) - { - _resCache = null; - if (_defaultReader != null) - { - _defaultReader.Close(); - _defaultReader = null; - } - _caseInsensitiveTable = null; - // Set Reader to null to avoid a race in GetObject. - base.Dispose(disposing); - } - } - else - { - // Just to make sure we always clear these fields in the future... - _resCache = null; - _caseInsensitiveTable = null; - _defaultReader = null; - base.Dispose(disposing); + _defaultReader?.Close(); } + + _defaultReader = null; + _resCache = null; + _caseInsensitiveTable = null; + base.Dispose(disposing); } public override IDictionaryEnumerator GetEnumerator() @@ -275,14 +236,13 @@ IEnumerator IEnumerable.GetEnumerator() private IDictionaryEnumerator GetEnumeratorHelper() { - IResourceReader copyOfReader = Reader; - if (copyOfReader == null || _resCache == null) + ResourceReader? reader = _defaultReader; + if (reader is null) throw new ObjectDisposedException(null, SR.ObjectDisposed_ResourceSet); - return copyOfReader.GetEnumerator(); + return reader.GetEnumerator(); } - public override string? GetString(string key) { object? o = GetObject(key, false, true); @@ -309,158 +269,104 @@ private IDictionaryEnumerator GetEnumeratorHelper() { if (key == null) throw new ArgumentNullException(nameof(key)); - if (Reader == null || _resCache == null) + + ResourceReader? reader = _defaultReader; + Dictionary? cache = _resCache; + Dictionary? caseInsensitiveTable = _caseInsensitiveTable; + if (reader is null || cache is null) throw new ObjectDisposedException(null, SR.ObjectDisposed_ResourceSet); - object? value = null; - ResourceLocator resLocation; + object? value; + ResourceLocator resEntry; - lock (Reader) + lock (cache) { - if (Reader == null) - throw new ObjectDisposedException(null, SR.ObjectDisposed_ResourceSet); - - if (_defaultReader != null) + // Find the offset within the data section + int dataPos; + if (cache.TryGetValue(key, out resEntry)) { - // Find the offset within the data section - int dataPos = -1; - if (_resCache.TryGetValue(key, out resLocation)) - { - value = resLocation.Value; - dataPos = resLocation.DataPosition; - } + value = resEntry.Value; + if (value != null) + return value; - if (dataPos == -1 && value == null) - { - dataPos = _defaultReader.FindPosForResource(key); - } + // When data type cannot be cached + dataPos = resEntry.DataPosition; + return isString ? reader.LoadString(dataPos) : reader.LoadObject(dataPos, out _); + } - if (dataPos != -1 && value == null) - { - Debug.Assert(dataPos >= 0, "data section offset cannot be negative!"); - // Normally calling LoadString or LoadObject requires - // taking a lock. Note that in this case, we took a - // lock on the entire RuntimeResourceSet, which is - // sufficient since we never pass this ResourceReader - // to anyone else. - ResourceTypeCode typeCode; - if (isString) - { - value = _defaultReader.LoadString(dataPos); - typeCode = ResourceTypeCode.String; - } - else - { - value = _defaultReader.LoadObject(dataPos, out typeCode); - } - - resLocation = new ResourceLocator(dataPos, (ResourceLocator.CanCache(typeCode)) ? value : null); - lock (_resCache) - { - _resCache[key] = resLocation; - } - } + dataPos = reader.FindPosForResource(key); + if (dataPos >= 0) + { + value = ReadValue(reader, dataPos, isString, out resEntry); + cache[key] = resEntry; + return value; + } + } - if (value != null || !ignoreCase) - { - return value; // may be null - } - } // if (_defaultReader != null) + if (!ignoreCase) + { + return null; + } - // At this point, we either don't have our default resource reader - // or we haven't found the particular resource we're looking for - // and may have to search for it in a case-insensitive way. - if (!_haveReadFromReader) - { - // If necessary, init our case insensitive hash table. - if (ignoreCase) - { - _caseInsensitiveTable ??= new Dictionary(StringComparer.OrdinalIgnoreCase); - } + // We haven't found the particular resource we're looking for + // and may have to search for it in a case-insensitive way. + bool initialize = false; + if (caseInsensitiveTable == null) + { + caseInsensitiveTable = new Dictionary(StringComparer.OrdinalIgnoreCase); + initialize = true; + } - if (_defaultReader == null) - { - IDictionaryEnumerator en = Reader.GetEnumerator(); - while (en.MoveNext()) - { - DictionaryEntry entry = en.Entry; - string readKey = (string)entry.Key; - ResourceLocator resLoc = new ResourceLocator(-1, entry.Value); - _resCache.Add(readKey, resLoc); - if (ignoreCase) - { - Debug.Assert(_caseInsensitiveTable != null); - _caseInsensitiveTable.Add(readKey, resLoc); - } - } - // Only close the reader if it is NOT our default one, - // since we need it around to resolve ResourceLocators. - if (!ignoreCase) - Reader.Close(); - } - else - { - Debug.Assert(ignoreCase, "This should only happen for case-insensitive lookups"); - Debug.Assert(_caseInsensitiveTable != null); - ResourceReader.ResourceEnumerator en = _defaultReader.GetEnumeratorInternal(); - while (en.MoveNext()) - { - // Note: Always ask for the resource key before the data position. - string currentKey = (string)en.Key; - int dataPos = en.DataPosition; - ResourceLocator resLoc = new ResourceLocator(dataPos, null); - _caseInsensitiveTable.Add(currentKey, resLoc); - } - } - _haveReadFromReader = true; - } - object? obj = null; - bool found = false; - bool keyInWrongCase = false; - if (_defaultReader != null) - { - if (_resCache.TryGetValue(key, out resLocation)) - { - found = true; - obj = ResolveResourceLocator(resLocation, key, _resCache, keyInWrongCase); - } - } - if (!found && ignoreCase) + lock (caseInsensitiveTable) + { + if (initialize) { - Debug.Assert(_caseInsensitiveTable != null); - if (_caseInsensitiveTable.TryGetValue(key, out resLocation)) + ResourceReader.ResourceEnumerator en = reader.GetEnumeratorInternal(); + while (en.MoveNext()) { - found = true; - keyInWrongCase = true; - obj = ResolveResourceLocator(resLocation, key, _resCache, keyInWrongCase); + // The resource key must be read before the data position. + string currentKey = (string)en.Key; + ResourceLocator resLoc = new ResourceLocator(en.DataPosition, null); + caseInsensitiveTable.Add(currentKey, resLoc); } + + _caseInsensitiveTable = caseInsensitiveTable; } - return obj; - } // lock(Reader) + + if (!caseInsensitiveTable.TryGetValue(key, out resEntry)) + return null; + + if (resEntry.Value != null) + return resEntry.Value; + + value = ReadValue(reader, resEntry.DataPosition, isString, out resEntry); + + if (resEntry.Value != null) + caseInsensitiveTable[key] = resEntry; + } + + return value; } - // The last parameter indicates whether the lookup required a - // case-insensitive lookup to succeed, indicating we shouldn't add - // the ResourceLocation to our case-sensitive cache. - private object? ResolveResourceLocator(ResourceLocator resLocation, string key, Dictionary copyOfCache, bool keyInWrongCase) + private static object? ReadValue (ResourceReader reader, int dataPos, bool isString, out ResourceLocator locator) { - // We need to explicitly resolve loosely linked manifest - // resources, and we need to resolve ResourceLocators with null objects. - object? value = resLocation.Value; - if (value == null) + object? value; + ResourceTypeCode typeCode; + + // Normally calling LoadString or LoadObject requires + // taking a lock. Note that in this case, we took a + // lock before calling this method. + if (isString) { - ResourceTypeCode typeCode; - lock (Reader) - { - Debug.Assert(_defaultReader != null); - value = _defaultReader.LoadObject(resLocation.DataPosition, out typeCode); - } - if (!keyInWrongCase && ResourceLocator.CanCache(typeCode)) - { - resLocation.Value = value; - copyOfCache[key] = resLocation; - } + value = reader.LoadString(dataPos); + typeCode = ResourceTypeCode.String; } + else + { + value = reader.LoadObject(dataPos, out typeCode); + } + + locator = new ResourceLocator(dataPos, ResourceLocator.CanCache(typeCode) ? value : null); return value; } } diff --git a/src/libraries/System.Resources.Extensions/tests/BinaryResourceWriterUnitTest.cs b/src/libraries/System.Resources.Extensions/tests/BinaryResourceWriterUnitTest.cs index c6061a82de2b8..34c3faa83a309 100644 --- a/src/libraries/System.Resources.Extensions/tests/BinaryResourceWriterUnitTest.cs +++ b/src/libraries/System.Resources.Extensions/tests/BinaryResourceWriterUnitTest.cs @@ -470,7 +470,7 @@ public static void ResourceManagerLoadsCorrectReader() { ResourceManager resourceManager = new ResourceManager(typeof(TestData)); ResourceSet resSet = resourceManager.GetResourceSet(CultureInfo.InvariantCulture, true, true); - IResourceReader reader = (IResourceReader)resSet.GetType().GetProperty("Reader", BindingFlags.NonPublic | BindingFlags.Instance)?.GetValue(resSet); + IResourceReader reader = (IResourceReader)resSet.GetType().GetField("_defaultReader", BindingFlags.NonPublic | BindingFlags.Instance)?.GetValue(resSet); Assert.IsType(reader); } diff --git a/src/libraries/System.Resources.ResourceManager/tests/ResourceManagerTests.cs b/src/libraries/System.Resources.ResourceManager/tests/ResourceManagerTests.cs index 9c038b540e12a..2bedd241197c3 100644 --- a/src/libraries/System.Resources.ResourceManager/tests/ResourceManagerTests.cs +++ b/src/libraries/System.Resources.ResourceManager/tests/ResourceManagerTests.cs @@ -54,6 +54,8 @@ public static void GetString_Basic(string key, string expectedValue) ResourceManager resourceManager = new ResourceManager("System.Resources.Tests.Resources.TestResx", typeof(ResourceManagerTests).GetTypeInfo().Assembly); string actual = resourceManager.GetString(key); Assert.Equal(expectedValue, actual); + Assert.Same(actual, resourceManager.GetString(key)); + Assert.Equal(expectedValue, resourceManager.GetObject(key)); } [Theory] @@ -297,6 +299,8 @@ public static void GetResourceSet_Strings(string key, string expectedValue) var culture = new CultureInfo("en-US"); ResourceSet set = manager.GetResourceSet(culture, true, true); Assert.Equal(expectedValue, set.GetString(key)); + Assert.Equal(expectedValue, set.GetObject(key)); + Assert.Equal(expectedValue, set.GetString(key)); } [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsBinaryFormatterSupported))] @@ -308,6 +312,19 @@ public static void GetResourceSet_NonStrings(string key, object expectedValue, b var culture = new CultureInfo("en-US"); ResourceSet set = manager.GetResourceSet(culture, true, true); Assert.Equal(expectedValue, set.GetObject(key)); + Assert.Equal(expectedValue, set.GetObject(key)); + } + + [ConditionalTheory(typeof(PlatformDetection), nameof(PlatformDetection.IsBinaryFormatterSupported))] + [MemberData(nameof(EnglishNonStringResourceData))] + public static void GetResourceSet_NonStringsIgnoreCase(string key, object expectedValue, bool requiresBinaryFormatter) + { + _ = requiresBinaryFormatter; + var manager = new ResourceManager("System.Resources.Tests.Resources.TestResx.netstandard17", typeof(ResourceManagerTests).GetTypeInfo().Assembly); + var culture = new CultureInfo("en-US"); + ResourceSet set = manager.GetResourceSet(culture, true, true); + Assert.Equal(expectedValue, set.GetObject(key.ToLower(), true)); + Assert.Equal(expectedValue, set.GetObject(key.ToLower(), true)); } [ConditionalTheory(Helpers.IsDrawingSupported)] @@ -385,6 +402,18 @@ public static void ConstructorNonRuntimeAssembly() Assert.Throws(() => new ResourceManager("name", assembly, null)); } + [Fact] + public static void GetStringAfterDispose() + { + var manager = new ResourceManager("System.Resources.Tests.Resources.TestResx", typeof(ResourceManagerTests).GetTypeInfo().Assembly); + var culture = new CultureInfo("en-US"); + ResourceSet set = manager.GetResourceSet(culture, true, true); + + set.GetString("Any"); + ((IDisposable)set).Dispose(); + Assert.Throws (() => set.GetString("Any")); + } + private class MockAssembly : Assembly { }