Skip to content

Conversation

@daxian-dbw
Copy link
Member

@daxian-dbw daxian-dbw commented Jun 11, 2020

PR Summary

Fix #1581

Before this change, WriteHistoryRange reads the new content from the history file and populate the new items to the history queue before the writing.
This is fine when the circular history queue is not full, but when the queue is full, new items will push the existing items in the queue forward, and thus the start to end range will be pointing to different items in the queue. This is causing the bug reported in #1581

The fix is to read the new content from history file before the writing, but defer populating the history queue until after the writing is done (no matter the writing failed or succeeded).

This change also avoids calling WithHistoryFileMutexDo within a WithHistoryFileMutexDo. The nested WithHistoryFileMutexDo call causes the mutex to be released before the outer WithHistoryFileMutexDo finishes. I'm not familiar with the recursive semantics of Mutex, so I'm not sure if this is a true problem, but I did experience mutex issues on Windows, where all PowerShell sessions failed to grab the mutex in the call _historyFileMutex.WaitOne(timeout), and all history lookup operations became super slow. I hope that can be fixed by avoiding nested WithHistoryFileMutexDo calls.

It may be easier to review by ignoring white space characters:
https://github.com/PowerShell/PSReadLine/pull/1602/files?w=1

PR Checklist

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • Make sure you've added one or more new tests
  • User-facing changes
    • Not Applicable
    • OR
    • Documentation needed at PowerShell-Docs
      • Doc Issue filed:
Microsoft Reviewers: Open in CodeFlow

@daxian-dbw daxian-dbw requested a review from lzybkr June 11, 2020 18:57
Copy link
Contributor

@lzybkr lzybkr left a comment

Choose a reason for hiding this comment

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

I don't think this fixes #211, but it might make it less apparent.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

When more than one Powershell instance open, PSReadLine does not save history correctly

2 participants