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

Subject: Re: [boost] [process] Formal Review starts today, 27 October
From: Gavin Lambert (gavinl_at_[hidden])
Date: 2016-11-01 03:15:04

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.

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.