Fix the NREs when writing to console from multiple threads #25440
Conversation
|
@iSazonov Could you re-add the backport consider labels? Or it does not matter. |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
@kborowinski Thanks for your contribution! |
|
@PowerShell/powershell-maintainers triage decision: |
|
I understand the decision not to backport this PR to the LTS version, but I would like to clarify that the actual script results are affected in PS 7.4.X and up. I first reported this issue on the 7Zip4PowerShell repo, where the unpacking process was crashing mid-task. try {
worker.Progress.StatusDescription = "Finished";
worker.Progress.RecordType = ProgressRecordType.Completed;
WriteProgress(worker.Progress);
} catch (NullReferenceException) {
// Possible bug in PowerShell 7.4.0 leading to a null reference exception being thrown on ProgressPane completion
// This is not happening on PowerShell 5.1
}This issue affects all scripts that write to the console from multiple threads, leading to potential crashes in the LTS version of PowerShell. Affected users are forced to disable the progress bar either globally or per script. |
…l#25440) The WriteImpl() method should always be called within a lock on _instanceLock to ensure thread safety.
…l#25440) The WriteImpl() method should always be called within a lock on _instanceLock to ensure thread safety.
|
@kborowinski I had the bot open PRs for the backports and added your comments. These still need to be triaged, but we are moving over to a new process that helps us track all the information we need. |
|
|
@TravisEz13 In comment on the original issue @iSazonov pinpointed it to PR #13758 |
Root CauseI debugged the code and found that the race condition was caused by The NRE happens because Looking closely, The root cause becomes clear give the above -- calling So, the potential problem still exists in CosnoleHostUserInterface.PreRead(), ConsoleHostUserInterface.PostWrite(), and ConsoleHostUserInterface.PostRead(). The latter 2 call Thoughts on this fixThe safest and complete fix would be adding However, after discussing with @SeeminglyScience and thinking about this more, we decide to not make further changes to the original fix. Here is why:
|
@daxian-dbw Thank you for your investigations! The reason I didn't dare to do this was for fear of a deadlock. There are too many combinations. Even a change in one place in the future could potentially open up scenarios impossible today. |
|
@SeeminglyScience did look into the feasibility to make the progress rendering lock-less, using atomic primitives. But it turned out to be a too big effort and too low priority for the team to commit on. |
PR Summary
Fix #21021
(Reopening this small PR on a new dev branch after closure due to the GitHub issue confirmed by support.)
This PR adds additional locks to prevent
NullReferenceExceptionwhen writing to the console from multiple threads. By ensuring proper synchronization, it improves the stability and reliability of concurrent console output in PowerShell.PR Context
The issue #21021 was caused by race conditions where multiple threads attempted to write to the console simultaneously, leading to potential
NullReferenceExceptions. This PR introduces additional locking mechanisms to prevent such occurrences and ensure thread-safe console access.I would like to thank @iSazonov for finding the cause and proposing the solution.
Visuals
On the left with the fix, on the right current state:
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright headerWIP:or[ WIP ]to the beginning of the title (theWIPbot will keep its status check atPendingwhile the prefix is present) and remove the prefix when the PR is ready.- [ ] Issue filed:
(which runs in a different PS Host).