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

[Chris Wilson]

Hi Charles,

On Mon, 26 Mar 2007, Charles Lecklider 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.
>>
>> 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.

So far I haven't had any problems using overlapped I/O for read only. What 
kind of strange behaviour might I get?

>> 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.

No, you get deadlocks if you perform simultaneous operations on the pipe 
in multiple threads, e.g. Write() in one thread while there is ablocked 
Read() happening in the other. This used to happen to me quite often until 
I moved all the reads and writes to happen in the same thread, and never 
since.

> 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.

The pipe thread can't use blocking I/O because it also has to listen for 
events from the main threads which signal that it should write to the 
pipe. I couldn't find a way to wait on both a filehandle and an event, and 
I didn't want to create a separate pipe just to communicate between 
threads, so I changed the pipe thread to use only non-blocking I/O and 
events.

> 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?

No, there are only two threads, the main one and the pipe handler.

> 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.

Does that still apply after my explanation above?

> 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?

Not at all, all of this is on the client side (bbackupd). Nothing at all 
to do with the Win32 server, which doesn't use WinNamedPipeStream at all.

Cheers, Chris.
-- 
_____ __     _
\  __/ / ,__(_)_  | Chris Wilson <0000 at qwirx.com> - Cambs UK |
/ (_/ ,\/ _/ /_ \ | Security/C/C++/Java/Perl/SQL/HTML Developer |
\ _/_/_/_//_/___/ | We are GNU-free your mind-and your software |