[Box Backup-commit] Re: #3: Merge Win32 branch

[Charles Lecklider]

Chris Wilson wrote:
> Hi Charles,
> 
> On Sun, 25 Mar 2007, trac at fluffy.co.uk wrote:
> 
>> [1486] I don't know if I can do this, but I'm going to try :-)
>>
>> I'd like to veto this change. You seem to have done a partial re-
>> implementation of a completion port - see CreateIoCompletionPort and
>> friends.
> 
> Thanks for the suggestion, but I'm afraid that after reading the
> CreateIoCompletionPort docs I'm still not completely sure how I could
> use it to replace [1486].
> 
> So far, what I understand (I think) is that the command socket thread,
> instead of calling WaitForMultipleObjects, would call
> GetQueuedCompletionStatus, which blocks until either (i) the main thread
> posts a message to the port with PostQueuedCompletionStatus, or (ii) a
> packet is received over the command socket by the pending overlapped
> I/O. Is that correct?

Or until the pipe is closed, yes.

> What bothers me is that MSDN says "After an instance of an open file is
> associated with an I/O completion port, it cannot be used in the
> ReadFileEx or WriteFileEx function."
> 
> I assume that this means that I have to use overlapped I/O for both
> reading and writing from the pipe?

Erm, you have to do that anyway or you'll get some really strange behaviour.

> I don't care about completion of
> writes, although I suppose I could dispatch the next waiting message (if
> any) to the pipe in this case.

Trying to cut corners when using overlapped IO is not a good idea.

> And how do I actually read or write on the pipe, if ReadFileEx and
> WriteFileEx are forbidden? Do good old ReadFile and WriteFile work
> properly?

Yes. You can't use ReadFileEx because you can't use completion routines
with completion ports (kinda makes sense when you think about it).

> I have a feeling that implementing this will move the WinNamedPipeStream
> API so far away from POSIX that I will have to create a new class that
> doesn't extend IOStream for this purpose. While it might be a cleaner
> solution to the current overlapped/nonoverlapped read branches in the
> code, I'm worried about the duplication of code and possible maintenance
> headaches.

Well, I'm still trying to work out what you're hoping to achieve; the
change talks about deadlock, but this is a pipe - that can't happen if
you always check for errors.

Obviously there needs to be a thread to deal with commands, but if
there's only one thread there's no harm in using normal boring
synchronous IO. The actual backup sockets ought to be using IOCP, but
that's a completely different issue.

However, it looks like you're using another worker thread as well in
order to do the actual work. If there's only one connection to the
command pipe, why is this thread needed? There's nothing to synchronise,
so why not do it as part of the pipe handling?

If you really need another thread, just create an IOCP, not bind
anything to it, and PostQueuedCompletionStatus at it. Loop on
GetQueuedCompletionStatus in the other thread and you're done. No
critsec, events, or lists needed.


IIRC the Win32 server part was just to allow the tests to run. This
seems like a lot of work for that. Maybe I'm missing something?

-C