git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Johan Herland <johan@herland.net>
Cc: Junio C Hamano <gitster@pobox.com>,
	Shawn Pearce <spearce@spearce.org>,
	git@vger.kernel.org, Johannes Sixt <j6t@kdbg.org>
Subject: Re: [PATCHv3 2/9] send-pack: Attempt to retrieve remote status even if pack-objects fails
Date: Mon, 16 May 2011 00:07:45 -0400	[thread overview]
Message-ID: <20110516040745.GA23583@sigill.intra.peff.net> (raw)
In-Reply-To: <1305495440-30836-3-git-send-email-johan@herland.net>

On Sun, May 15, 2011 at 11:37:13PM +0200, Johan Herland wrote:

> I first wrote this patch on a base where e07fd15 (Peff's "send-pack:
> unbreak push over stateless rpc") was not present, and then resolved
> a conflict when rebasing this patch onto current master. I hope Peff
> or Johannes (Sixt) can verify that my patch does not reintroduce the
> deadlock they fixed.

I don't think it reintroduces the deadlock we fixed, but I am worried
that it produces a new, similar one. That is, imagine pack-objects fails
horribly, maybe or maybe not producing any output. We close its pipe
outgoing pipe at the end of run_command, and per Johannes' 09c9957, we
are sure that the sideband demuxer does not hold a pipe end open,
either.

So the remote side sees us close our end of the pipe, knows there is no
more pack data, and then closes their end. So our receive_status should
either get some error message, or EOF, either of which is fine. So no
deadlock there. Essentially, we have done a half-duplex shutdown of the
connection to the remote, and that is enough for everybody to keep
going.

But what if we are not using pipes, but have an actual TCP socket? In
that case, I'm not sure what happens. We don't seem to do a half-duplex
shutdown() anywhere. So I'm concerned that we are still open for sending
from the remote's perspective, and we may deadlock.

However, that would not necessarily be something introduced by your
patch; you would deadlock in receive_status, but prior to that it would
deadlock in the sideband demuxer.

AFAICT, the only way to have an actual TCP connection instead of pipes
is for the push to go over git://, which is enabled almost nowhere. But
we should perhaps check for deadlock on failed pack-objects in that
case, both with and without your patch.

-Peff

  reply	other threads:[~2011-05-16  4:07 UTC|newest]

Thread overview: 53+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-05-13 16:54 [PATCH 2/2] receive-pack: Add receive.denyObjectLimit to refuse push with too many objects Johan Herland
2011-05-13 17:09 ` Junio C Hamano
2011-05-14  1:43   ` Johan Herland
2011-05-14  2:03     ` [PATCHv2 2/2] receive-pack: Add receive.objectCountLimit " Johan Herland
2011-05-14  2:30       ` Shawn Pearce
2011-05-14 13:17         ` Johan Herland
2011-05-14 22:17           ` Shawn Pearce
2011-05-15 17:42             ` Johan Herland
2011-05-15 21:37               ` [PATCHv3 0/9] Push limits Johan Herland
2011-05-15 21:37                 ` [PATCHv3 1/9] Update technical docs to reflect side-band-64k capability in receive-pack Johan Herland
2011-05-15 21:37                 ` [PATCHv3 2/9] send-pack: Attempt to retrieve remote status even if pack-objects fails Johan Herland
2011-05-16  4:07                   ` Jeff King [this message]
2011-05-16  6:13                     ` Jeff King
2011-05-16  6:39                       ` Jeff King
2011-05-16  6:46                         ` [PATCH 1/3] connect: treat generic proxy processes like ssh processes Jeff King
2011-05-16 19:57                           ` Johannes Sixt
2011-05-16 23:12                             ` Junio C Hamano
2011-05-17  5:54                             ` Jeff King
2011-05-17 20:14                               ` Johannes Sixt
2011-05-18  8:57                                 ` Jeff King
2011-05-16  6:52                         ` [PATCH 2/3] connect: let callers know if connection is a socket Jeff King
2011-05-16  6:52                         ` [PATCH 3/3] send-pack: avoid deadlock on git:// push with failed pack-objects Jeff King
2011-05-16 20:02                           ` Johannes Sixt
2011-05-17  5:56                             ` Jeff King
2011-05-18 20:24                               ` [PATCH] Windows: add a wrapper for the shutdown() system call Johannes Sixt
2011-05-15 21:37                 ` [PATCHv3 3/9] pack-objects: Allow --max-pack-size to be used together with --stdout Johan Herland
2011-05-15 22:06                   ` Shawn Pearce
2011-05-16  1:39                     ` Johan Herland
2011-05-16  6:12                   ` Junio C Hamano
2011-05-16  9:27                     ` Johan Herland
2011-05-15 21:37                 ` [PATCHv3 4/9] pack-objects: Teach new option --max-object-count, similar to --max-pack-size Johan Herland
2011-05-15 22:07                   ` Shawn Pearce
2011-05-15 22:31                     ` Johan Herland
2011-05-15 23:48                       ` Shawn Pearce
2011-05-16  6:25                     ` Junio C Hamano
2011-05-16  9:49                       ` Johan Herland
2011-05-15 21:37                 ` [PATCHv3 5/9] pack-objects: Teach new option --max-commit-count, limiting #commits in pack Johan Herland
2011-05-15 21:37                 ` [PATCHv3 6/9] receive-pack: Prepare for addition of the new 'limit-*' family of capabilities Johan Herland
2011-05-16  6:50                   ` Junio C Hamano
2011-05-16  9:53                     ` Johan Herland
2011-05-16 22:02                     ` Sverre Rabbelier
2011-05-16 22:07                       ` Junio C Hamano
2011-05-16 22:09                         ` Sverre Rabbelier
2011-05-16 22:12                           ` Junio C Hamano
2011-05-16 22:16                             ` Sverre Rabbelier
2011-05-15 21:37                 ` [PATCHv3 7/9] send-pack/receive-pack: Allow server to refuse pushes with too many objects Johan Herland
2011-05-15 21:37                 ` [PATCHv3 8/9] send-pack/receive-pack: Allow server to refuse pushing too large packs Johan Herland
2011-05-15 21:37                 ` [PATCHv3 9/9] send-pack/receive-pack: Allow server to refuse pushes with too many commits Johan Herland
2011-05-15 21:52                 ` [PATCHv3 0/9] Push limits Ævar Arnfjörð Bjarmason
2011-05-14 17:50         ` [PATCHv2 2/2] receive-pack: Add receive.objectCountLimit to refuse push with too many objects Junio C Hamano
2011-05-14 22:27           ` Shawn Pearce
2011-05-13 18:20 ` [PATCH 2/2] receive-pack: Add receive.denyObjectLimit " Johannes Sixt
2011-05-14  1:49   ` Johan Herland

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20110516040745.GA23583@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=j6t@kdbg.org \
    --cc=johan@herland.net \
    --cc=spearce@spearce.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).