Re: [boost] [process] Formal Review starts today, 27 October

Subject: Re: [boost] [process] Formal Review starts today, 27 October
From: Klemens Morgenstern (klemens.morgenstern_at_[hidden])
Date: 2016-11-01 06:59:58

Am 01.11.2016 um 08:15 schrieb Gavin Lambert:
> On 1/11/2016 13:34, Klemens Morgenstern wrote:
>> No, I am not. Since you have streams, they must have a handle so I need
>> to be able to duplicate pipes. Hence I could just close a duplicate,
>> which doesn't do a thing to help with the problem.
>
> If you're creating a stream around the handle then the stream should 
> own the handle -- you can duplicate it if you need to (eg. to change 
> inheritance or security) but then close the original. Or to put it a 
> different way, the handle should be moved into the stream, not copied.
>
> It's generally a bad idea to have more than one handle pointing at the 
> same thing.
It's unavoidable for design reasons as given in this case:

pipe p;
opstream os(p);
ipstream is(p);

os << 42;
int i;
is >> i;

And I cannot only pass half of the pipe to opstream or ipstream, because:

pipe p;
ipstream is(p);
child c("foo", std_out> is);//here I need the handle

>
> So for stdout for example, you get a "read handle" and a "write 
> handle" when the pipe is constructed.  You move the read handle to the 
> ipstream, and the write handle to the STARTUPINFO; after CreateProcess 
> returns (and before you try to use the read handle) you close the 
> write handle. Now the only handle to the pipe that the parent has is 
> the read handle in the ipstream.  No duplication.
>
>
> Here is some very dumb code that demonstrates pipes working as expected:
>
>     LPCWSTR name = L"\\\\.\\pipe\\testpipe";
>     HANDLE hRead = CreateNamedPipeW(name,
>                                     PIPE_ACCESS_INBOUND | 
> FILE_FLAG_FIRST_PIPE_INSTANCE | FILE_FLAG_OVERLAPPED,
>                                     PIPE_TYPE_BYTE | 
> PIPE_READMODE_BYTE | PIPE_WAIT,
>                                     1, 0, 0, 10000, NULL);
>     assert(hRead != INVALID_HANDLE_VALUE);
>
>     SECURITY_ATTRIBUTES inherit = { sizeof(SECURITY_ATTRIBUTES), NULL, 
> TRUE };
>     HANDLE hWrite = CreateFileW(name, GENERIC_WRITE | SYNCHRONIZE, 0, 
> &inherit, OPEN_EXISTING, FILE_ATTRIBUTE_NORMAL, NULL);
>     assert(hWrite != INVALID_HANDLE_VALUE);
>
>     STARTUPINFOW si = { sizeof(STARTUPINFOW) };
>     si.dwFlags = STARTF_USESTDHANDLES;
>     si.hStdOutput = hWrite;
>     PROCESS_INFORMATION pi;
>     wchar_t cmdLine[] = L"cmd /c ver";
>     assert(FALSE != CreateProcessW(NULL, cmdLine, NULL, NULL, TRUE, 
> CREATE_NO_WINDOW, NULL, NULL, &si, &pi));
>
>     CloseHandle(hWrite);
>     CloseHandle(pi.hThread);
>
>     OVERLAPPED ovl = { 0 };
>     ovl.hEvent = CreateEvent(NULL, TRUE, TRUE, NULL);
>     char buffer[2048];
>     DWORD len;
>
>     bool reading = false;
>     while (WaitForSingleObject(pi.hProcess, 0) != WAIT_OBJECT_0)
>     {
>         if (!reading)
>         {
>             ReadFile(hRead, buffer, sizeof(buffer), NULL, &ovl);
>             reading = true;
>         }
>         if (WaitForSingleObject(ovl.hEvent, 100) == WAIT_OBJECT_0)
>         {
>             reading = false;
>             if (GetOverlappedResult(hRead, &ovl, &len, FALSE))
>             {
>                 std::cout << std::string(buffer, len);
>             }
>             else
>             {
>                 DWORD error = GetLastError();
>                 if (error != ERROR_BROKEN_PIPE)
>                 {
>                     std::cerr << "Error " << GetLastError() << std::endl;
>                 }
>                 break;
>             }
>         }
>     }
>     if (reading)
>     {
>         if (GetOverlappedResult(hRead, &ovl, &len, TRUE))
>         {
>             std::cout << std::string(buffer, len);
>         }
>     }
>     CloseHandle(pi.hProcess);
>     CloseHandle(ovl.hEvent);
>     CloseHandle(hRead);
>
> So if you're not getting the same behaviour, then you've probably lost 
> a handle somewhere that needed to be closed (perhaps the move bugs?). 
> This code won't deadlock if the process finishes, although since it's 
> faking asynchrony it will deadlock if it does a blocking wait for 
> termination without finishing the read.
>
> Perhaps the most important observation is that if you comment out the 
> CloseHandle(hWrite), then it *will* deadlock.
>
Alright I'm starting to doubt my sanity. I'm quite certain I tested 
that, even before I've written the async_pipe class etc. and I tried to 
get some clear information out of the msdn about that. And I thought 
this was strange behaviour, but only for the named pipes. I have no clue 
what I did wrong, because I could reproduce what you said using 
asnyc_pipe just now.

Nevertheless, I am very happy I've been wrong, thank you so much. That 
is the on part where I wasn't very confident about the library, I'll add 
the feature after the review, since that also needs a lot of testing.

So the behaviour that will be implemented then is quite simply: if you 
use a pipe or pstream with a process the unused side will be closed 
(also on error) and if necessary (on posix) it'll set some flags.

If you want to avoid the automatic closing for whatever reason, you can 
just duplicate the pipe:

ipstream p;
pipe p2 = p.pipe(); //duplicate
child c("foo", std_out > p2);

int i;
p >> i; //going for the deadlock

And that would of course also mean, that you'd have a close pipe after this:

pipe p;
child c1("foo", std_out>p);
child c2("bar", std_in < p);

It'll also be closed on error, so we have consistent behaviour, if some 
decides to do that:

std::error_code ec;
ipstream is;
child c("invalid-file", std_out > is, ec);

int i;
is >>  i;

I hope that makes sense, if the library passes the review, I will fix 
that before it goes into boost.