-
Notifications
You must be signed in to change notification settings - Fork 7.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enable Add-Content
to share read access to other tools while writing content
#8091
Enable Add-Content
to share read access to other tools while writing content
#8091
Conversation
924280e
to
777f23f
Compare
@@ -59,3 +59,37 @@ Describe "Add-Content cmdlet tests" -Tags "CI" { | |||
} | |||
} | |||
} | |||
|
|||
Describe "Add-Content feature tests" -Tag Feature { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We always have problems with async tests. Do we really need the test? I'd only add an comment in C# code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kind of agree on async tests being inherently fragile. Perhaps for this one a comment in the code is sufficient.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can make it sync:
- open a file with
FileStream(FileAccess.Read, FileShare.ReadWrite)
and keep it open. - run new
Add-Content
(withFileAccess.Write, FileShare.ReadWrite
) - close
FileStream
from 1). The test should not throw.
The old Add-Content
(with FileShare.Write
) in 2) would throw.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good suggestion, let me see if that repros the original issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see if these changes are possible. It would harden the test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@anmenaga that worked perfectly and so simple :)
$writer = Start-Job -ScriptBlock { | ||
param($logpath) | ||
|
||
1..10 | % { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use ForEach-Object
instead of %
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is (1..10).foreach{...}
faster then ForEach-Object
?
PR Summary
Add-Content
uses a writer that currently sets share access to write. So if using a tool liketail
to read a log file while trying to write to it will fail. Fix is to change share access to readwrite which will allow other processes to continue to read from it while the writer writes new content.Fix #5924
PR Checklist
.h
,.cpp
,.cs
,.ps1
and.psm1
files have the correct copyright headerWIP:
to the beginning of the title and remove the prefix when the PR is ready.[feature]
if the change is significant or affects feature tests