From 425449453c0ba3637457674d58aa312e0d817e6a Mon Sep 17 00:00:00 2001 From: Ilya Date: Tue, 29 Jan 2019 15:01:35 +0500 Subject: [PATCH 1/5] Reduce race condition probability for config file access --- .../engine/PSConfiguration.cs | 186 +++++++++--------- 1 file changed, 97 insertions(+), 89 deletions(-) diff --git a/src/System.Management.Automation/engine/PSConfiguration.cs b/src/System.Management.Automation/engine/PSConfiguration.cs index d12f5616de7..229c8068025 100644 --- a/src/System.Management.Automation/engine/PSConfiguration.cs +++ b/src/System.Management.Automation/engine/PSConfiguration.cs @@ -380,7 +380,7 @@ internal PSKeyword GetLogKeywords() /// The type of the value /// The ConfigScope of the configuration file to update. /// The string key of the value. - /// The default value to return if the key is not present. + /// The default value to return if the key is not present or the config file is not available. /// private T ReadValueFromFile(ConfigScope scope, string key, T defaultValue = default(T), Func readImpl = null) @@ -389,28 +389,34 @@ internal PSKeyword GetLogKeywords() if (!File.Exists(fileName)) { return defaultValue; } // Open file for reading, but allow multiple readers - fileLock.EnterReadLock(); - try + if (fileLock.TryEnterReadLock(millisecondsTimeout: 10)) { - // The config file can be locked by another process - // so we wait some milliseconds in 'WaitForFile()' for recovery before stop current process. - using (var readerStream = WaitForFile(fileName, FileMode.Open, FileAccess.Read, FileShare.ReadWrite)) - using (var streamReader = new StreamReader(readerStream)) - using (var jsonReader = new JsonTextReader(streamReader)) + try { - var settings = new JsonSerializerSettings() { TypeNameHandling = TypeNameHandling.None, MaxDepth = 10 }; - var serializer = JsonSerializer.Create(settings); - - var configData = serializer.Deserialize(jsonReader); - if (configData != null && configData.TryGetValue(key, StringComparison.OrdinalIgnoreCase, out JToken jToken)) + // The config file can be locked by another process + // so we wait some milliseconds in 'WaitForFile()' for recovery before stop current process. + using (var readerStream = WaitForFile(fileName, FileMode.Open, FileAccess.Read, FileShare.ReadWrite)) + using (var streamReader = new StreamReader(readerStream)) + using (var jsonReader = new JsonTextReader(streamReader)) { - return readImpl != null ? readImpl(jToken, serializer, defaultValue) : jToken.ToObject(serializer); + var settings = new JsonSerializerSettings() { TypeNameHandling = TypeNameHandling.None, MaxDepth = 10 }; + var serializer = JsonSerializer.Create(settings); + + var configData = serializer.Deserialize(jsonReader); + if (configData != null && configData.TryGetValue(key, StringComparison.OrdinalIgnoreCase, out JToken jToken)) + { + return readImpl != null ? readImpl(jToken, serializer, defaultValue) : jToken.ToObject(serializer); + } } } - } - finally - { - fileLock.ExitReadLock(); + catch (IOException) + { + return defaultValue; + } + finally + { + fileLock.ExitReadLock(); + } } return defaultValue; @@ -418,7 +424,7 @@ internal PSKeyword GetLogKeywords() private FileStream WaitForFile(string fullPath, FileMode mode, FileAccess access, FileShare share) { - const int MaxTries = 5; + const int MaxTries = 20; for (int numTries = 0; numTries < MaxTries; numTries++) { try @@ -432,7 +438,7 @@ private FileStream WaitForFile(string fullPath, FileMode mode, FileAccess access throw; } - Thread.Sleep(50); + Thread.Sleep(20); } } @@ -450,97 +456,99 @@ private FileStream WaitForFile(string fullPath, FileMode mode, FileAccess access private void UpdateValueInFile(ConfigScope scope, string key, T value, bool addValue) { string fileName = GetConfigFilePath(scope); - fileLock.EnterWriteLock(); - try + if (fileLock.TryEnterWriteLock(millisecondsTimeout: 50)) { - // Since multiple properties can be in a single file, replacement - // is required instead of overwrite if a file already exists. - // Handling the read and write operations within a single FileStream - // prevents other processes from reading or writing the file while - // the update is in progress. It also locks out readers during write - // operations. - using (FileStream fs = new FileStream(fileName, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None)) + try { - JObject jsonObject = null; - - // UTF8, BOM detection, and bufferSize are the same as the basic stream constructor. - // The most important parameter here is the last one, which keeps the StreamReader - // (and FileStream) open during Dispose so that it can be reused for the write - // operation. - using (StreamReader streamRdr = new StreamReader(fs, Encoding.UTF8, true, 1024, true)) - using (JsonTextReader jsonReader = new JsonTextReader(streamRdr)) + // Since multiple properties can be in a single file, replacement + // is required instead of overwrite if a file already exists. + // Handling the read and write operations within a single FileStream + // prevents other processes from reading or writing the file while + // the update is in progress. It also locks out readers during write + // operations. + using (FileStream fs = WaitForFile(fileName, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None)) { - // Safely determines whether there is content to read from the file - bool isReadSuccess = jsonReader.Read(); - if (isReadSuccess) + JObject jsonObject = null; + + // UTF8, BOM detection, and bufferSize are the same as the basic stream constructor. + // The most important parameter here is the last one, which keeps the StreamReader + // (and FileStream) open during Dispose so that it can be reused for the write + // operation. + using (StreamReader streamRdr = new StreamReader(fs, Encoding.UTF8, true, 1024, true)) + using (JsonTextReader jsonReader = new JsonTextReader(streamRdr)) { - // Read the stream into a root JObject for manipulation - jsonObject = (JObject) JToken.ReadFrom(jsonReader); - JProperty propertyToModify = jsonObject.Property(key); - - if (propertyToModify == null) + // Safely determines whether there is content to read from the file + bool isReadSuccess = jsonReader.Read(); + if (isReadSuccess) { - // The property doesn't exist, so add it - if (addValue) + // Read the stream into a root JObject for manipulation + jsonObject = (JObject) JToken.ReadFrom(jsonReader); + JProperty propertyToModify = jsonObject.Property(key); + + if (propertyToModify == null) + { + // The property doesn't exist, so add it + if (addValue) + { + jsonObject.Add(new JProperty(key, value)); + } + // else the property doesn't exist so there is nothing to remove + } + // The property exists + else { - jsonObject.Add(new JProperty(key, value)); + if (addValue) + { + propertyToModify.Replace(new JProperty(key, value)); + } + else + { + propertyToModify.Remove(); + } } - // else the property doesn't exist so there is nothing to remove } - // The property exists else { + // The file doesn't already exist and we want to write to it + // or it exists with no content. + // A new file will be created that contains only this value. + // If the file doesn't exist and a we don't want to write to it, no + // action is necessary. if (addValue) { - propertyToModify.Replace(new JProperty(key, value)); + jsonObject = new JObject(new JProperty(key, value)); } else { - propertyToModify.Remove(); + return; } } } - else - { - // The file doesn't already exist and we want to write to it - // or it exists with no content. - // A new file will be created that contains only this value. - // If the file doesn't exist and a we don't want to write to it, no - // action is necessary. - if (addValue) - { - jsonObject = new JObject(new JProperty(key, value)); - } - else - { - return; - } - } - } - // Reset the stream position to the beginning so that the - // changes to the file can be written to disk - fs.Seek(0, SeekOrigin.Begin); + // Reset the stream position to the beginning so that the + // changes to the file can be written to disk + fs.Seek(0, SeekOrigin.Begin); - // Update the file with new content - using (StreamWriter streamWriter = new StreamWriter(fs)) - using (JsonTextWriter jsonWriter = new JsonTextWriter(streamWriter)) - { - // The entire document exists within the root JObject. - // I just need to write that object to produce the document. - jsonObject.WriteTo(jsonWriter); - - // This trims the file if the file shrank. If the file grew, - // it is a no-op. The purpose is to trim extraneous characters - // from the file stream when the resultant JObject is smaller - // than the input JObject. - fs.SetLength(fs.Position); + // Update the file with new content + using (StreamWriter streamWriter = new StreamWriter(fs)) + using (JsonTextWriter jsonWriter = new JsonTextWriter(streamWriter)) + { + // The entire document exists within the root JObject. + // I just need to write that object to produce the document. + jsonObject.WriteTo(jsonWriter); + + // This trims the file if the file shrank. If the file grew, + // it is a no-op. The purpose is to trim extraneous characters + // from the file stream when the resultant JObject is smaller + // than the input JObject. + fs.SetLength(fs.Position); + } } } - } - finally - { - fileLock.ExitWriteLock(); + finally + { + fileLock.ExitWriteLock(); + } } } From 1caaa7cd4b0bb00a879ee85a95944cf51e86af1d Mon Sep 17 00:00:00 2001 From: Ilya Date: Fri, 22 Feb 2019 11:38:40 +0500 Subject: [PATCH 2/5] Revert TryEnterReadLock --- .../engine/PSConfiguration.cs | 44 +++++++++---------- 1 file changed, 21 insertions(+), 23 deletions(-) diff --git a/src/System.Management.Automation/engine/PSConfiguration.cs b/src/System.Management.Automation/engine/PSConfiguration.cs index 229c8068025..b09818b3fc2 100644 --- a/src/System.Management.Automation/engine/PSConfiguration.cs +++ b/src/System.Management.Automation/engine/PSConfiguration.cs @@ -389,34 +389,32 @@ internal PSKeyword GetLogKeywords() if (!File.Exists(fileName)) { return defaultValue; } // Open file for reading, but allow multiple readers - if (fileLock.TryEnterReadLock(millisecondsTimeout: 10)) + fileLock.EnterReadLock(); + try { - try + // The config file can be locked by another process + // so we wait some milliseconds in 'WaitForFile()' for recovery before stop current process. + using (var readerStream = WaitForFile(fileName, FileMode.Open, FileAccess.Read, FileShare.ReadWrite)) + using (var streamReader = new StreamReader(readerStream)) + using (var jsonReader = new JsonTextReader(streamReader)) { - // The config file can be locked by another process - // so we wait some milliseconds in 'WaitForFile()' for recovery before stop current process. - using (var readerStream = WaitForFile(fileName, FileMode.Open, FileAccess.Read, FileShare.ReadWrite)) - using (var streamReader = new StreamReader(readerStream)) - using (var jsonReader = new JsonTextReader(streamReader)) - { - var settings = new JsonSerializerSettings() { TypeNameHandling = TypeNameHandling.None, MaxDepth = 10 }; - var serializer = JsonSerializer.Create(settings); + var settings = new JsonSerializerSettings() { TypeNameHandling = TypeNameHandling.None, MaxDepth = 10 }; + var serializer = JsonSerializer.Create(settings); - var configData = serializer.Deserialize(jsonReader); - if (configData != null && configData.TryGetValue(key, StringComparison.OrdinalIgnoreCase, out JToken jToken)) - { - return readImpl != null ? readImpl(jToken, serializer, defaultValue) : jToken.ToObject(serializer); - } + var configData = serializer.Deserialize(jsonReader); + if (configData != null && configData.TryGetValue(key, StringComparison.OrdinalIgnoreCase, out JToken jToken)) + { + return readImpl != null ? readImpl(jToken, serializer, defaultValue) : jToken.ToObject(serializer); } } - catch (IOException) - { - return defaultValue; - } - finally - { - fileLock.ExitReadLock(); - } + } + catch (IOException) + { + return defaultValue; + } + finally + { + fileLock.ExitReadLock(); } return defaultValue; From 7381a1d8154076a55d61d28a776bc17cd8af0b40 Mon Sep 17 00:00:00 2001 From: Ilya Date: Fri, 22 Feb 2019 22:59:14 +0500 Subject: [PATCH 3/5] Revert TryEnterWriteLock --- .../engine/PSConfiguration.cs | 146 +++++++++--------- 1 file changed, 69 insertions(+), 77 deletions(-) diff --git a/src/System.Management.Automation/engine/PSConfiguration.cs b/src/System.Management.Automation/engine/PSConfiguration.cs index b09818b3fc2..e230b4c4c00 100644 --- a/src/System.Management.Automation/engine/PSConfiguration.cs +++ b/src/System.Management.Automation/engine/PSConfiguration.cs @@ -454,98 +454,90 @@ private FileStream WaitForFile(string fullPath, FileMode mode, FileAccess access private void UpdateValueInFile(ConfigScope scope, string key, T value, bool addValue) { string fileName = GetConfigFilePath(scope); - if (fileLock.TryEnterWriteLock(millisecondsTimeout: 50)) + fileLock.EnterWriteLock(); + + // Since multiple properties can be in a single file, replacement + // is required instead of overwrite if a file already exists. + // Handling the read and write operations within a single FileStream + // prevents other processes from reading or writing the file while + // the update is in progress. It also locks out readers during write + // operations. + using (FileStream fs = WaitForFile(fileName, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None)) { - try + JObject jsonObject = null; + + // UTF8, BOM detection, and bufferSize are the same as the basic stream constructor. + // The most important parameter here is the last one, which keeps the StreamReader + // (and FileStream) open during Dispose so that it can be reused for the write + // operation. + using (StreamReader streamRdr = new StreamReader(fs, Encoding.UTF8, true, 1024, true)) + using (JsonTextReader jsonReader = new JsonTextReader(streamRdr)) { - // Since multiple properties can be in a single file, replacement - // is required instead of overwrite if a file already exists. - // Handling the read and write operations within a single FileStream - // prevents other processes from reading or writing the file while - // the update is in progress. It also locks out readers during write - // operations. - using (FileStream fs = WaitForFile(fileName, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None)) + // Safely determines whether there is content to read from the file + bool isReadSuccess = jsonReader.Read(); + if (isReadSuccess) { - JObject jsonObject = null; - - // UTF8, BOM detection, and bufferSize are the same as the basic stream constructor. - // The most important parameter here is the last one, which keeps the StreamReader - // (and FileStream) open during Dispose so that it can be reused for the write - // operation. - using (StreamReader streamRdr = new StreamReader(fs, Encoding.UTF8, true, 1024, true)) - using (JsonTextReader jsonReader = new JsonTextReader(streamRdr)) + // Read the stream into a root JObject for manipulation + jsonObject = (JObject) JToken.ReadFrom(jsonReader); + JProperty propertyToModify = jsonObject.Property(key); + + if (propertyToModify == null) { - // Safely determines whether there is content to read from the file - bool isReadSuccess = jsonReader.Read(); - if (isReadSuccess) + // The property doesn't exist, so add it + if (addValue) { - // Read the stream into a root JObject for manipulation - jsonObject = (JObject) JToken.ReadFrom(jsonReader); - JProperty propertyToModify = jsonObject.Property(key); - - if (propertyToModify == null) - { - // The property doesn't exist, so add it - if (addValue) - { - jsonObject.Add(new JProperty(key, value)); - } - // else the property doesn't exist so there is nothing to remove - } - // The property exists - else - { - if (addValue) - { - propertyToModify.Replace(new JProperty(key, value)); - } - else - { - propertyToModify.Remove(); - } - } + jsonObject.Add(new JProperty(key, value)); + } + // else the property doesn't exist so there is nothing to remove + } + // The property exists + else + { + if (addValue) + { + propertyToModify.Replace(new JProperty(key, value)); } else { - // The file doesn't already exist and we want to write to it - // or it exists with no content. - // A new file will be created that contains only this value. - // If the file doesn't exist and a we don't want to write to it, no - // action is necessary. - if (addValue) - { - jsonObject = new JObject(new JProperty(key, value)); - } - else - { - return; - } + propertyToModify.Remove(); } } - - // Reset the stream position to the beginning so that the - // changes to the file can be written to disk - fs.Seek(0, SeekOrigin.Begin); - - // Update the file with new content - using (StreamWriter streamWriter = new StreamWriter(fs)) - using (JsonTextWriter jsonWriter = new JsonTextWriter(streamWriter)) + } + else + { + // The file doesn't already exist and we want to write to it + // or it exists with no content. + // A new file will be created that contains only this value. + // If the file doesn't exist and a we don't want to write to it, no + // action is necessary. + if (addValue) { - // The entire document exists within the root JObject. - // I just need to write that object to produce the document. - jsonObject.WriteTo(jsonWriter); - - // This trims the file if the file shrank. If the file grew, - // it is a no-op. The purpose is to trim extraneous characters - // from the file stream when the resultant JObject is smaller - // than the input JObject. - fs.SetLength(fs.Position); + jsonObject = new JObject(new JProperty(key, value)); + } + else + { + return; } } } - finally + + // Reset the stream position to the beginning so that the + // changes to the file can be written to disk + fs.Seek(0, SeekOrigin.Begin); + + // Update the file with new content + using (StreamWriter streamWriter = new StreamWriter(fs)) + using (JsonTextWriter jsonWriter = new JsonTextWriter(streamWriter)) { - fileLock.ExitWriteLock(); + // The entire document exists within the root JObject. + // I just need to write that object to produce the document. + jsonObject.WriteTo(jsonWriter); + + // This trims the file if the file shrank. If the file grew, + // it is a no-op. The purpose is to trim extraneous characters + // from the file stream when the resultant JObject is smaller + // than the input JObject. + fs.SetLength(fs.Position); } } } From 3ddd12bd95a91e90e309e58fc18115927353a398 Mon Sep 17 00:00:00 2001 From: Ilya Date: Thu, 7 Mar 2019 08:57:00 +0500 Subject: [PATCH 4/5] Revert default and retries --- .../engine/PSConfiguration.cs | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/src/System.Management.Automation/engine/PSConfiguration.cs b/src/System.Management.Automation/engine/PSConfiguration.cs index e230b4c4c00..d9a98b2483e 100644 --- a/src/System.Management.Automation/engine/PSConfiguration.cs +++ b/src/System.Management.Automation/engine/PSConfiguration.cs @@ -408,10 +408,6 @@ internal PSKeyword GetLogKeywords() } } } - catch (IOException) - { - return defaultValue; - } finally { fileLock.ExitReadLock(); @@ -422,7 +418,7 @@ internal PSKeyword GetLogKeywords() private FileStream WaitForFile(string fullPath, FileMode mode, FileAccess access, FileShare share) { - const int MaxTries = 20; + const int MaxTries = 5; for (int numTries = 0; numTries < MaxTries; numTries++) { try @@ -436,7 +432,7 @@ private FileStream WaitForFile(string fullPath, FileMode mode, FileAccess access throw; } - Thread.Sleep(20); + Thread.Sleep(50); } } From fee0cbbb8bbfec25d288d9e68cbe7f25da41d17e Mon Sep 17 00:00:00 2001 From: Ilya Date: Thu, 7 Mar 2019 10:15:30 +0500 Subject: [PATCH 5/5] Revert comment and try-finally --- .../engine/PSConfiguration.cs | 136 +++++++++--------- 1 file changed, 71 insertions(+), 65 deletions(-) diff --git a/src/System.Management.Automation/engine/PSConfiguration.cs b/src/System.Management.Automation/engine/PSConfiguration.cs index d9a98b2483e..630bda9333f 100644 --- a/src/System.Management.Automation/engine/PSConfiguration.cs +++ b/src/System.Management.Automation/engine/PSConfiguration.cs @@ -380,7 +380,7 @@ internal PSKeyword GetLogKeywords() /// The type of the value /// The ConfigScope of the configuration file to update. /// The string key of the value. - /// The default value to return if the key is not present or the config file is not available. + /// The default value to return if the key is not present. /// private T ReadValueFromFile(ConfigScope scope, string key, T defaultValue = default(T), Func readImpl = null) @@ -451,91 +451,97 @@ private void UpdateValueInFile(ConfigScope scope, string key, T value, bool a { string fileName = GetConfigFilePath(scope); fileLock.EnterWriteLock(); - - // Since multiple properties can be in a single file, replacement - // is required instead of overwrite if a file already exists. - // Handling the read and write operations within a single FileStream - // prevents other processes from reading or writing the file while - // the update is in progress. It also locks out readers during write - // operations. - using (FileStream fs = WaitForFile(fileName, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None)) + try { - JObject jsonObject = null; - - // UTF8, BOM detection, and bufferSize are the same as the basic stream constructor. - // The most important parameter here is the last one, which keeps the StreamReader - // (and FileStream) open during Dispose so that it can be reused for the write - // operation. - using (StreamReader streamRdr = new StreamReader(fs, Encoding.UTF8, true, 1024, true)) - using (JsonTextReader jsonReader = new JsonTextReader(streamRdr)) + // Since multiple properties can be in a single file, replacement + // is required instead of overwrite if a file already exists. + // Handling the read and write operations within a single FileStream + // prevents other processes from reading or writing the file while + // the update is in progress. It also locks out readers during write + // operations. + using (FileStream fs = WaitForFile(fileName, FileMode.OpenOrCreate, FileAccess.ReadWrite, FileShare.None)) { - // Safely determines whether there is content to read from the file - bool isReadSuccess = jsonReader.Read(); - if (isReadSuccess) + JObject jsonObject = null; + + // UTF8, BOM detection, and bufferSize are the same as the basic stream constructor. + // The most important parameter here is the last one, which keeps the StreamReader + // (and FileStream) open during Dispose so that it can be reused for the write + // operation. + using (StreamReader streamRdr = new StreamReader(fs, Encoding.UTF8, true, 1024, true)) + using (JsonTextReader jsonReader = new JsonTextReader(streamRdr)) { - // Read the stream into a root JObject for manipulation - jsonObject = (JObject) JToken.ReadFrom(jsonReader); - JProperty propertyToModify = jsonObject.Property(key); - - if (propertyToModify == null) + // Safely determines whether there is content to read from the file + bool isReadSuccess = jsonReader.Read(); + if (isReadSuccess) { - // The property doesn't exist, so add it - if (addValue) + // Read the stream into a root JObject for manipulation + jsonObject = (JObject) JToken.ReadFrom(jsonReader); + JProperty propertyToModify = jsonObject.Property(key); + + if (propertyToModify == null) { - jsonObject.Add(new JProperty(key, value)); + // The property doesn't exist, so add it + if (addValue) + { + jsonObject.Add(new JProperty(key, value)); + } + // else the property doesn't exist so there is nothing to remove + } + // The property exists + else + { + if (addValue) + { + propertyToModify.Replace(new JProperty(key, value)); + } + else + { + propertyToModify.Remove(); + } } - // else the property doesn't exist so there is nothing to remove } - // The property exists else { + // The file doesn't already exist and we want to write to it + // or it exists with no content. + // A new file will be created that contains only this value. + // If the file doesn't exist and a we don't want to write to it, no + // action is necessary. if (addValue) { - propertyToModify.Replace(new JProperty(key, value)); + jsonObject = new JObject(new JProperty(key, value)); } else { - propertyToModify.Remove(); + return; } } } - else - { - // The file doesn't already exist and we want to write to it - // or it exists with no content. - // A new file will be created that contains only this value. - // If the file doesn't exist and a we don't want to write to it, no - // action is necessary. - if (addValue) - { - jsonObject = new JObject(new JProperty(key, value)); - } - else - { - return; - } - } - } - // Reset the stream position to the beginning so that the - // changes to the file can be written to disk - fs.Seek(0, SeekOrigin.Begin); + // Reset the stream position to the beginning so that the + // changes to the file can be written to disk + fs.Seek(0, SeekOrigin.Begin); - // Update the file with new content - using (StreamWriter streamWriter = new StreamWriter(fs)) - using (JsonTextWriter jsonWriter = new JsonTextWriter(streamWriter)) - { - // The entire document exists within the root JObject. - // I just need to write that object to produce the document. - jsonObject.WriteTo(jsonWriter); - - // This trims the file if the file shrank. If the file grew, - // it is a no-op. The purpose is to trim extraneous characters - // from the file stream when the resultant JObject is smaller - // than the input JObject. - fs.SetLength(fs.Position); + // Update the file with new content + using (StreamWriter streamWriter = new StreamWriter(fs)) + using (JsonTextWriter jsonWriter = new JsonTextWriter(streamWriter)) + { + // The entire document exists within the root JObject. + // I just need to write that object to produce the document. + jsonObject.WriteTo(jsonWriter); + + // This trims the file if the file shrank. If the file grew, + // it is a no-op. The purpose is to trim extraneous characters + // from the file stream when the resultant JObject is smaller + // than the input JObject. + fs.SetLength(fs.Position); + } } } + finally + { + fileLock.ExitWriteLock(); + } } ///