-
Notifications
You must be signed in to change notification settings - Fork 4.6k
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
Modified #41008 without GetOverlappedResultEx #41236
Conversation
Tagging subscribers to this area: @tommcdon |
coreclr pri0 Win-x64 failure is #40916 These failures don't look related to these changes; I'll rerun these two jobs. |
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.
LGTM, thanks @josalem!
src/libraries/System.Private.CoreLib/src/System/WeakReference.cs
Outdated
Show resolved
Hide resolved
032e692
to
691983d
Compare
Hello @josalem! Because this pull request has the p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
* Revert "Revert "Improve Windows error handling in Diagnostics IPC (dotnet#41008)" (dotnet#41220)" This reverts commit 15839c0. * don't use win8+ API
fixes #41217
Same changes as #41008 but without using
GetOverlappedResultEx
which is only available in Windows 8 forward. This modification does not change the behavior of the patch.Below is the PR description from #41008.
fixes #41007
These changes are intended to be backported to 5.0-rc1 once merged into master.
As described in #41007, the runtime was treating
ERROR_BROKEN_PIPE
as a generic error rather than a hangup error. These changes correct that misbehavior and reset the connection on generic errors in case additional errors have been missed. Additionally, they simplify the logic ofIpcStream::{Read|Write}
to useGetOverlappedResultEx
which directly takes a timeout rather thanGetOverlappedResult
followed byWaitForSingleObject
.CC @jander-msft @tommcdon
Customer Impact
This currently impacts
dotnet-monitor
on Windows.If a Connect Diagnostic Port on Windows doesn't call
DisconnectNamedPipe
before closing it will cause runtimes to be unable to connect to another Connect Diagnostic Port at the same address. For example,dotnet-monitor
currently does not callDisconnectNamedPipe
before closing, so if a user restartsdotnet-monitor
they won't see any apps show up if they connected before.Testing
I have tested these changes locally with
dotnet-monitor
both under a debugger and without the debugger. These fix the observed issue. I did not add an individual test for this scenario, but one could be created.Risk
This is a low risk patch. It simply improves the error handling and corrects a misbehavior.