Re: [PATCH] Fix potential local deadlock during fetch-pack

From: Shawn Pearce <spearce@spearce.org>
Date: Mar 29, 2011, 17:22 UTC
Message-ID: <AANLkTikZ=POeQi5nnt8D9v4Z-_OaXNrXDjP2awWUtKzA@mail.gmail.com>
In-Reply-To: <7vtyelsvp0.fsf@alter.siamese.dyndns.org>

On Tue, Mar 29, 2011 at 10:06, Junio C Hamano <gitster@pobox.com> wrote:

> The fetch-pack/upload-pack protocol relies on the underlying transport
> (local pipe or TCP socket) to have enough slack to allow one window worth
> of data in flight without blocking the writer.  Traditionally we always
> relied on being able to have a batch of 32 "have"s in flight (roughly 1.5k

Its 64. Because the client "races ahead" one window before ever reading. Which is closer to 3K of data in flight.

> The recent "progressive-stride" change allows "fetch-pack" to send up to
> 1024 "have"s without reading any response from "upload-pack".  The
> outgoing pipe of "upload-pack" can be clogged with many ACK and NAK that
> are unread, while "fetch-pack" is still stuffing its outgoing pike with

s/pike/pipe/

Show 19 quoted lines

> @@ -229,16 +229,17 @@ static void insert_alternate_refs(void)
>  }
>
>  #define INITIAL_FLUSH 16
> +#define PIPESAFE_FLUSH 32
>  #define LARGE_FLUSH 1024
>
>  static int next_flush(int count)
>  {
> -       if (count < INITIAL_FLUSH * 2)
> -               count += INITIAL_FLUSH;
> -       else if (count < LARGE_FLUSH)
> +       int flush_limit = args.stateless_rpc ? LARGE_FLUSH : PIPESAFE_FLUSH;
> +
> +       if (count < flush_limit)
>                count <<= 1;
>        else
> -               count += LARGE_FLUSH;
> +               count += flush_limit;

Nak. You still deadlock because when count reaches PIPESAFE_FLUSH you still double it to 2*PIPESAFE_FLUSH here. Instead I think you mean:

  if (args.stateless_rpc) {
    if (count < LARGE_FLUSH)
      count <<= 1;
    else
      count += LARGE_FLUSH;
  } else {
    if (count * 2 < PIPESAFE_FLUSH)
      count <<= 1;
  }

-- 
Shawn.