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.