Re: [msysGit] [PATCH/RFC 06/11] run-command: add kill_async() and is_async_alive()

[Erik Faye-Lund <kusmabite@googlemail.com>]

On Fri, Nov 27, 2009 at 8:59 PM, Johannes Sixt <j6t@kdbg.org> wrote:
> On Freitag, 27. November 2009, Erik Faye-Lund wrote:
>> Do you really think it's better to unconditionally take down the
>> entire process with an error, instead of having a relatively small
>> chance of stuff blowing up without any sensible error? I'm not 100%
>> convinced - but let's hope we'll find a proper fix.
>
> "relatively small chance of stuff blowing up"? The docs of
> TerminateThread: "... the kernel32 state for the thread's process could be
> inconsistent." That's scary if we are talking about a process that should run
> for days or weeks without interruption.
>

I think there's a misunderstanding here. I thought your suggestion was
to simply call die(), which would take down the main process. After
reading this explanation, I think you're talking about giving an error
and rejecting the connection instead. Which makes more sense than to
risk crashing the main-process, indeed.

> The reason why we are killing a thread is to prevent keeping lots of
> connections open (to the same IP address). There are two situations to take
> care of:
>
> 1. We are in a lengthy computation without paying attention to the socket.
>
> 2. The client does not send or accept data for a long time.
>
> Case 1 could happen if upload-pack is "counting objects" on a large
> repository. We would need some way to kill upload-pack. Since it is a
> separate process anyway, we could use TerminateProcess().
>

Makes sense. I'll play around a bit with this and see what I come up with.

> Case 2 could be achieved by using setsockopt() with SO_RCVTIMEO and
> SO_SNDTIMEO and a tiny timeout. But notice that we would set a timeout in one
> thread while another thread is waiting in ReadFile() or WriteFile(). Would
> that work?
>

I think it should work fine, but I won't give you a guarantee ;)
Perhaps we should have a configurable global max timeout, and just set
that on all sockets? Or does this open for DDOS attacks?

Anyway, thanks for the sanity :)

-- 
Erik "kusma" Faye-Lund