Fix incremental history saving when the history queue is full (#1602)

PSReadLine/History.cs

@@ -257,12 +257,12 @@ private void IncrementalHistoryWrite()
                 i -= 1;
             }
 
-            WriteHistoryRange(i + 1, _history.Count - 1, File.AppendText);
+            WriteHistoryRange(i + 1, _history.Count - 1, overwritten: false);
         }
 
         private void SaveHistoryAtExit()
         {
-            WriteHistoryRange(0, _history.Count - 1, File.CreateText);
+            WriteHistoryRange(0, _history.Count - 1, overwritten: true);
         }
 
         private int historyErrorReportedCount;
@@ -321,70 +321,99 @@ private bool WithHistoryFileMutexDo(int timeout, Action action)
             return true;
         }
 
-        private void WriteHistoryRange(int start, int end, Func<string, StreamWriter> fileOpener)
+        private void WriteHistoryRange(int start, int end, bool overwritten)
         {
             WithHistoryFileMutexDo(100, () =>
             {
-                if (!MaybeReadHistoryFile())
-                    return;
-
                 bool retry = true;
-                retry_after_creating_directory:
+                // Get the new content since the last sync.
+                List<string> historyLines = overwritten ? null : ReadHistoryFileIncrementally();
+
                 try
                 {
-                    using (var file = fileOpener(Options.HistorySavePath))
+                    retry_after_creating_directory:
+                    try
                     {
-                        for (var i = start; i <= end; i++)
+                        using (var file = overwritten ? File.CreateText(Options.HistorySavePath) : File.AppendText(Options.HistorySavePath))
                         {
-                            HistoryItem item = _history[i];
-                            item._saved = true;
+                            for (var i = start; i <= end; i++)
+                            {
+                                HistoryItem item = _history[i];
+                                item._saved = true;
 
-                            // Actually, skip writing sensitive items to file.
-                            if (item._sensitive) { continue; }
+                                // Actually, skip writing sensitive items to file.
+                                if (item._sensitive) { continue; }
 
-                            var line = item.CommandLine.Replace("\n", "`\n");
-                            file.WriteLine(line);
+                                var line = item.CommandLine.Replace("\n", "`\n");
+                                file.WriteLine(line);
+                            }
+                        }
+                        var fileInfo = new FileInfo(Options.HistorySavePath);
+                        _historyFileLastSavedSize = fileInfo.Length;
+                    }
+                    catch (DirectoryNotFoundException)
+                    {
+                        // Try making the directory, but just once
+                        if (retry)
+                        {
+                            retry = false;
+                            Directory.CreateDirectory(Path.GetDirectoryName(Options.HistorySavePath));
+                            goto retry_after_creating_directory;
                         }
                     }
-                    var fileInfo = new FileInfo(Options.HistorySavePath);
-                    _historyFileLastSavedSize = fileInfo.Length;
                 }
-                catch (DirectoryNotFoundException)
+                finally
                 {
-                    // Try making the directory, but just once
-                    if (retry)
+                    if (historyLines != null)
                     {
-                        retry = false;
-                        Directory.CreateDirectory(Path.GetDirectoryName(Options.HistorySavePath));
-                        goto retry_after_creating_directory;
+                        // Populate new history from other sessions to the history queue after we are done
+                        // with writing the specified range to the file.
+                        // We do it at this point to make sure the range of history items from 'start' to
+                        // 'end' do not get changed before the writing to the file.
+                        UpdateHistoryFromFile(historyLines, fromDifferentSession: true, fromInitialRead: false);
                     }
                 }
             });
         }
 
+        /// <summary>
+        /// Helper method to read the incremental part of the history file.
+        /// Note: the call to this method should be guarded by the mutex that protects the history file.
+        /// </summary>
+        private List<string> ReadHistoryFileIncrementally()
+        {
+            var fileInfo = new FileInfo(Options.HistorySavePath);
+            if (fileInfo.Exists && fileInfo.Length != _historyFileLastSavedSize)
+            {
+                var historyLines = new List<string>();
+                using (var fs = new FileStream(Options.HistorySavePath, FileMode.Open))
+                using (var sr = new StreamReader(fs))
+                {
+                    fs.Seek(_historyFileLastSavedSize, SeekOrigin.Begin);
+
+                    while (!sr.EndOfStream)
+                    {
+                        historyLines.Add(sr.ReadLine());
+                    }
+                }
+
+                _historyFileLastSavedSize = fileInfo.Length;
+                return historyLines.Count > 0 ? historyLines : null;
+            }
+
+            return null;
+        }
+
         private bool MaybeReadHistoryFile()
         {
             if (Options.HistorySaveStyle == HistorySaveStyle.SaveIncrementally)
             {
                 return WithHistoryFileMutexDo(1000, () =>
                 {
-                    var fileInfo = new FileInfo(Options.HistorySavePath);
-                    if (fileInfo.Exists && fileInfo.Length != _historyFileLastSavedSize)
+                    List<string> historyLines = ReadHistoryFileIncrementally();
+                    if (historyLines != null)
                     {
-                        var historyLines = new List<string>();
-                        using (var fs = new FileStream(Options.HistorySavePath, FileMode.Open))
-                        using (var sr = new StreamReader(fs))
-                        {
-                            fs.Seek(_historyFileLastSavedSize, SeekOrigin.Begin);
-
-                            while (!sr.EndOfStream)
-                            {
-                                historyLines.Add(sr.ReadLine());
-                            }
-                        }
                         UpdateHistoryFromFile(historyLines, fromDifferentSession: true, fromInitialRead: false);
-
-                        _historyFileLastSavedSize = fileInfo.Length;
                     }
                 });
             }

test/HistoryTest.cs

@@ -2,6 +2,7 @@
 using System.IO;
 using System.Linq;
 using System.Management.Automation;
+using System.Reflection;
 using Microsoft.PowerShell;
 using Xunit;
 
@@ -33,6 +34,60 @@ public void History()
             Test("dir c*", Keys(_.UpArrow, _.UpArrow, _.DownArrow));
         }
 
+        [SkippableFact]
+        public void ParallelHistorySaving()
+        {
+            TestSetup(KeyMode.Cmd);
+
+            string historySavingFile = Path.GetTempFileName();
+            var options = new SetPSReadLineOption {
+                HistorySaveStyle = HistorySaveStyle.SaveIncrementally,
+                MaximumHistoryCount = 3,
+            };
+
+            typeof(SetPSReadLineOption)
+                .GetField("_historySavePath", BindingFlags.Instance | BindingFlags.NonPublic)
+                .SetValue(options, historySavingFile);
+
+            PSConsoleReadLine.SetOptions(options);
+
+            // Set the initial history items.
+            string[] initialHistoryItems = new[] { "gcm help", "dir ~" };
+            SetHistory(initialHistoryItems);
+
+            // The initial history items should be saved to file.
+            string[] text = File.ReadAllLines(historySavingFile);
+            Assert.Equal(initialHistoryItems.Length, text.Length);
+            for (int i = 0; i < text.Length; i++)
+            {
+                Assert.Equal(initialHistoryItems[i], text[i]);
+            }
+
+            // Add another line to the file to mimic the new history saving from a different session.
+            using (var file = File.AppendText(historySavingFile))
+            {
+                file.WriteLine("cd Downloads");
+            }
+
+            PSConsoleReadLine.AddToHistory("cd Documents");
+
+            string[] expectedSavedLines = new[] { "gcm help", "dir ~", "cd Downloads", "cd Documents" };
+            text = File.ReadAllLines(historySavingFile);
+            Assert.Equal(expectedSavedLines.Length, text.Length);
+            for (int i = 0; i < text.Length; i++)
+            {
+                Assert.Equal(expectedSavedLines[i], text[i]);
+            }
+
+            string[] expectedHistoryItems = new[] { "dir ~", "cd Documents", "cd Downloads" };
+            var historyItems = PSConsoleReadLine.GetHistoryItems();
+            Assert.Equal(expectedHistoryItems.Length, historyItems.Length);
+            for (int i = 0; i < historyItems.Length; i++)
+            {
+                Assert.Equal(expectedHistoryItems[i], historyItems[i].CommandLine);
+            }
+        }
+
         [SkippableFact]
         public void SensitiveHistoryDefaultBehavior()
         {