Skip to content
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

Update IpcStreamFactory state machine to handle being started on a thread that ends #43711

Merged
merged 2 commits into from
Nov 5, 2020

Conversation

josalem
Copy link
Contributor

@josalem josalem commented Oct 22, 2020

Fixes #39176

"The I/O operation has been aborted because of either a thread exit or an application request."

  • ERROR_OPERATION_ABORTED description [emphasis mine]

The first call to IpcStream::DiagnosticsIpc::Listen happens on the thread where EEStartupHelper executes. On Windows, if that thread ends, e.g., in a custom hosting scenario, it causes all Overlapped IO that started on that thread to be cancelled. This specifically includes the asynchronous call to ConnectNamedPipe in IpcStreamFactory state machine. This causes GetNextAvailableStream to go into an error state that would continually check the status of the aborted IO without attempting to reset itself. It always see ERROR_OPERATION_ABORTED and doesn't self-correct. This patch accounts for mid-state change IO cancellations by automatically resetting the Listen port on the error.

I reproduced the behavior from #39176 locally (12% CPU being consumed by the error state) and confirmed that this change fixed the issue (CPU is reported as 0% in the repro).

This patch also updates the write/read error code to only call GetOverlappedResults on infinite timeouts if the error from ReadFile/WriteFile was ERROR_IO_PENDING.

CC @tommcdon @lateralusX

@josalem josalem added this to the 6.0.0 milestone Oct 22, 2020
@josalem josalem requested review from noahfalk and sywhang October 22, 2020 00:40
@josalem josalem self-assigned this Oct 22, 2020
@ghost
Copy link

ghost commented Oct 22, 2020

Tagging subscribers to this area: @tommcdon
See info in area-owners.md if you want to be subscribed.

@josalem josalem requested a review from a team November 3, 2020 17:04
Copy link
Contributor

@sdmaclea sdmaclea left a comment

Choose a reason for hiding this comment

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

LGTM, but I don't completely understand the code.

Copy link
Contributor

@sywhang sywhang left a comment

Choose a reason for hiding this comment

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

LGTM modulo few things 👍

src/coreclr/src/vm/ipcstreamfactory.cpp Outdated Show resolved Hide resolved
@josalem josalem merged commit d65f284 into dotnet:master Nov 5, 2020
@josalem josalem deleted the dev/josalem/diagport-handle-aborts branch November 5, 2020 00:22
@josalem
Copy link
Contributor Author

josalem commented Nov 5, 2020

/backport to release/5.0

@github-actions
Copy link
Contributor

github-actions bot commented Nov 5, 2020

Started backporting to release/5.0: https://github.com/dotnet/runtime/actions/runs/346557926

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants