Fix hanging of NamedPipeClientStream when connecting with infinite timeout

[cranberry-clockworks]

Resolved Problem

Affected OS: Only Windows.

The NamedPipeClientStream could hang on connection forever while doesn't respond on a cancellation token. This happens when several conditions are met:

1. An underlying file for named pipe is created. It means a pipe server accept connection from a different pipe.
2. The second client tries to connect with infinite timeout to the server.
3. The internal system function WainNamedPipe is called by the second client.

After that the second client doesn't exit when passed cancellation token is passed. The situation is
reproduces in the implemented test. One could compare test results before and after the bug fix.

Explanation of the Error

First, when a timeout parameter is not passed the infinite timeout is used.

public Task ConnectAsync(CancellationToken cancellationToken)
{
    return ConnectAsync(Timeout.Infinite, cancellationToken);
}

After that private void ConnectInternal(int timeout, CancellationToken cancellationToken, int startTime) is called.

The Timeout.Infinite is resolved to the -1 integer value. Therefore in the ConnectInternal() method int waitTime is resolved to -1 because elpased is 0 at first call. And TryConnect(waitTime, cancellationToken) is called with waitTime as -1.

If one inspects implementation of the TryConnect method for Windows he could discover two things:

1. The passed cancellation token not used at all.
2. The timeout is passed to the system method WaitNamedPipe as integer value:

if (!Interop.Kernel32.WaitNamedPipe(_normalizedPipePath, timeout))
{
    errorCode = Marshal.GetLastPInvokeError();
    ...

But as MSDN states WaitNamedPipe has timeout parameter as DWORD which is really resolved to uint. And there are two special values for the timeout parameters:

1. NMPWAIT_USE_DEFAULT_WAIT (0x00000000) to used a timeout specified in the CreateNamedPipe function.
2. NMPWAIT_WAIT_FOREVER (0xffffffff) to wait infinite until the instance is available.

The passed timeout value to the TryConnect() as -1 of type int is marshalled to 0xffffffff as DWORD. And the TryConnect() never returns until the pipe will be broken. And therefore passed cancellation token doesn't cancel the operation.

Implemented Solution

It's not correct to calculate difference between the timeout and the elapsed time when the timeout is infinite. One could do it only if the timeout is finite and grater or equals to zero.

In case of infinite timeout one should use CancellationCheckInterval as it was designed at first place.

[ghost]

Tagging subscribers to this area: @dotnet/area-system-io
See info in area-owners.md if you want to be subscribed.

Issue Details

---

Resolved Problem

Affected OS: Only Windows.

The NamedPipeClientStream could hang on connection forever while doesn't respond on a cancellation token. This happens when several conditions are met:

1. An underlying file for named pipe is created. It means a pipe server accept connection from a different pipe.
2. The second client tries to connect with infinite timeout to the server.
3. The internal system function WainNamedPipe is called by the second client.

After that the second client doesn't exit when passed cancellation token is passed. The situation is
reproduces in the implemented test. One could compare test results before and after the bug fix.

Explanation of the Error

First, when a timeout parameter is not passed the infinite timeout is used.

public Task ConnectAsync(CancellationToken cancellationToken)
{
    return ConnectAsync(Timeout.Infinite, cancellationToken);
}

After that private void ConnectInternal(int timeout, CancellationToken cancellationToken, int startTime) is called.

The Timeout.Infinite is resolved to the -1 integer value. Therefore in the ConnectInternal() method int waitTime is resolved to -1 because elpased is 0 at first call. And TryConnect(waitTime, cancellationToken) is called with waitTime as -1.

If one inspects implementation of the TryConnect method for Windows he could discover two things:

1. The passed cancellation token not used at all.
2. The timeout is passed to the system method WaitNamedPipe as integer value:

if (!Interop.Kernel32.WaitNamedPipe(_normalizedPipePath, timeout))
{
    errorCode = Marshal.GetLastPInvokeError();
    ...

But as MSDN states WaitNamedPipe has timeout parameter as DWORD which is really resolved to uint. And there are two special values for the timeout parameters:

1. NMPWAIT_USE_DEFAULT_WAIT (0x00000000) to used a timeout specified in the CreateNamedPipe function.
2. NMPWAIT_WAIT_FOREVER (0xffffffff) to wait infinite until the instance is available.

The passed timeout value to the TryConnect() as -1 of type int is marshalled to 0xffffffff as DWORD. And the TryConnect() never returns until the pipe will be broken. And therefore passed cancellation token doesn't cancel the operation.

Implemented Solution

It's not correct to calculate difference between the timeout and the elapsed time when the timeout is infinite. One could do it only if the timeout is finite and grater or equals to zero.

In case of infinite timeout one should use CancellationCheckInterval as it was designed at first place.

Author:	cranberry-knight
Assignees:	-
Labels:	area-System.IO
Milestone:	-

[dnfadmin]

All CLA requirements met.

[cranberry-clockworks]

@jeffhandley
Hi! Can you please provide a feedback?

[GSPP]

I'm linking to an issue that I opened that I believe to be relevant:

• Support async NamedPipeServerStream.WaitForConnection for synchronous pipes #63536

In this issue, there's a lot of discussion around how difficult it is to cancel a named pipe operation.

It seems you (@cranberry-knight) would like the CancellationToken to be respected. The linked issue contains a pretty thorough discussion for why that is rather difficult to do.

[cranberry-clockworks]

@GSPP Thanks for linking the issue and providing more information.

In my opinion, if the API provides method with cancellation token it should be possible to use at
any time. That for a cancellation token is! And also passed timeout treated well and works as
expected in the NamedPipeClientStream.

The async-over-sync pattern is implemented by dividing wait time into small pieces to be able check
the passed cancellation token. I think the problem of that PR not in the design domain but just a
regular bug where timeout handled wrong.

[danmoseley]

@adamsitnik looks like this one needs a review from IO.

[adamsitnik]

@cranberry-knight big thanks for your contribution and apologies for the delayed review! PTAL at my comments

[adamsitnik]

LGTM, thank you for your contribution @cranberry-knight !

[adamsitnik]

the failures are unrelated, I am merging