Add spin wait to the desktop compilation service client #11709
Conversation
On .NET 4.5 the implementation for NamedPipeClientStream busy waits during the timeout period, blocking the running CPU. Fixes dotnet#10413.
| if (timeoutMs != Timeout.Infinite && waitTime <= 0) | ||
| { | ||
| Log($"Connecting to server timed out after {timeoutMs} ms"); | ||
| break; |
There was a problem hiding this comment.
When we break out of here, we won't be connected, but then we subsequently log "Named pipe '{0}' connected", and it looks like the code proceeds under the assumption that it is connected. Does that matter? If you're ok with that, you could also change the beginning of the loop from while (true) { cancellationToken.ThrowIfCancellationRequested(); ... } to while (!cancellationToken.IsCancellationRequested), as the code after the loop also does the check-and-throw.
There was a problem hiding this comment.
You might consider separating all of this new code out into its own ConnectWithSpinWait method, or something like that, which provides the same logical behavior as Connect (including the throw for timeout / cancellation / etc.) except with the added spin wait... then none of the rest of the logic needs to change, and the method could easily be swapped out if/when Connect is fixed on all supported platforms.
There was a problem hiding this comment.
@stephentoub No that should be a return null; instead of a break -- nice catch. I will add a test for this.
agocke
force-pushed
the
spin-in-compiler-server-wait
branch
from
4c0615c
to
0c04ad4
Compare
|
@stephentoub I believe I've addressed your comments. |
| break; | ||
| } | ||
| catch (Exception e) when (e is IOException || e is TimeoutException) | ||
| { |
There was a problem hiding this comment.
Connect can throw two exceptions on a time out?
There was a problem hiding this comment.
Yeah... TimeoutException is when it doesn't find an available pipe server. IOException happens when a server exists but it's serving another client and the current client times out.
|
LGTM |
| } | ||
| catch (Exception e) when (e is IOException || e is TimeoutException) | ||
| { | ||
| // Ignore timeout |
There was a problem hiding this comment.
It is not clear why are we ignoring IOException. Perhaps explain in the comment?
Also, would it be better to have two strongly typed catch clauses instead of using a filter?
There was a problem hiding this comment.
Yeah I should add an explanation for that. IOException can sometimes indicate timeout.
There was a problem hiding this comment.
Regarding two catch clauses: this clause addresses the single semantic case of a timeout, which the framework has unfortunately provided two types for, which I describe in the comment. I think it is clearer to collect this code in a single catch, as opposed to splitting it into two cases.
agocke
force-pushed
the
spin-in-compiler-server-wait
branch
from
7e75ee6
to
a1eaab2
Compare
|
Issues should be fixed and test cases added. |
|
/cc @MattGertz for ask-mode approval |
| waitTime = Math.Min(timeoutMs - elapsedMs, maxWaitIntervalMs); | ||
| if (waitTime <= 0) | ||
| { | ||
| return false; |
There was a problem hiding this comment.
What should happen with negative input that is not Timeout.Infinite? Should we try at least once? Or is such input not possible?
There was a problem hiding this comment.
There was a problem hiding this comment.
This timeout is even more constrained than I thought: it isn't set anywhere except in test code. In public endpoints it is always set to a constant positive value. I will add an assert that it is in the expected range.
|
LGTM |
1 similar comment
|
LGTM |
|
@jaredpar @stephentoub Sorry, I saw the approvals and merged this before you had finished reviewing. If you find anything more I should add let me know and I'll address it in another PR. |
|
Is there any way we can get this tested with the DotNetCompiler project in an Azure environment to make sure the implementation actually solves the problem I and several others were experiencing in Production? There were some assumptions in the conversations that make me nervous, and I'd hate to see this get into whatever release is next and have it still not solve the problem. I'm an MVP, if there are any ways I can help test this before it ships, please let me know. Thanks! |
|
@advancedrei If you have a production environment that you can test this on, the 1.1.0-beta1-60606-02 build of package |
|
You'll probably also need to set |
On .NET 4.5 the implementation for NamedPipeClientStream busy waits
during the timeout period, blocking the running CPU. Fixes #10413.
/cc @jaredpar @stephentoub @dotnet/roslyn-compiler