git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: "René Scharfe" <l.s.r@web.de>
Cc: 唐宇奕 <winglovet@gmail.com>, git@vger.kernel.org
Subject: Re: Bug report: orphaned pack-objects after killing upload-pack on [was: (no subject)]
Date: Fri, 20 Nov 2020 19:29:21 -0500
Message-ID: <20201121002921.GC353076@coredump.intra.peff.net> (raw)
In-Reply-To: <badf3777-3970-b714-3ad9-67d2f77f94a5@web.de>

On Fri, Nov 20, 2020 at 07:52:45PM +0100, René Scharfe wrote:

> Have you seen this working as you expect in an earlier version?
> 
> I suspect it's a matter of turning on some flags (patch below), but I
> have to admit I don't really know what I'm doing here -- and why this
> hasn't been done already.
> 
> René
> 
> ---
>  upload-pack.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/upload-pack.c b/upload-pack.c
> index 5dc8e1f844..e42dea26fa 100644
> --- a/upload-pack.c
> +++ b/upload-pack.c
> @@ -321,6 +321,8 @@ static void create_pack_file(struct upload_pack_data *pack_data,
>  	pack_objects.in = -1;
>  	pack_objects.out = -1;
>  	pack_objects.err = -1;
> +	pack_objects.clean_on_exit = 1;
> +	pack_objects.wait_after_clean = 1;

Yeah, clean_on_exit seems quite reasonable to me. I suspect nobody ever
really noticed, because as soon as pack-objects starts to write out the
pack, it will get SIGPIPE or EPIPE and die. But there is no point in
letting it chug on expensive object enumeration or delta compression if
upload-pack has already exited.

I don't know that wait_after_clean is necessary. We don't need to wait
for pack-objects to fail.

On the flip side, one of the reasons I added clean_on_exit long ago was
for the similar issue on the push side, which is even worse. Here we
might just waste some CPU, but on the push side we connect pack-objects
directly to the remote socket, so it could actually complete the push
(from the server's perspective) after the local git-push has died. Or at
least I think that was possible at one point; it might not be the case
any more.

I wrote this patch ages ago, and it is still sitting close to the bottom
(if not the bottom) of my todo stack, waiting to be investigated. ;)

-- >8 --
Subject: [PATCH] send-pack: kill pack-objects helper on signal or exit

We spawn an external pack-objects process to actually send
objects to the remote side. If we are killed by a signal
during this process, the pack-objects will keep running and
complete the push, which may surprise the user. We should
take it down when we go down.

Signed-off-by: Jeff King <peff@peff.net>
---
 send-pack.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/send-pack.c b/send-pack.c
index eb4a44270b..d2701bf35c 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -85,6 +85,7 @@ static int pack_objects(int fd, struct ref *refs, struct oid_array *extra, struc
 	po.in = -1;
 	po.out = args->stateless_rpc ? -1 : fd;
 	po.git_cmd = 1;
+	po.clean_on_exit = 1;
 	if (start_command(&po))
 		die_errno("git pack-objects failed");
 
-- 
2.29.2.730.g3e418f96ba


  reply	other threads:[~2020-11-21  0:31 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-19  8:18 唐宇奕
2020-11-20 18:52 ` Bug report: orphaned pack-objects after killing upload-pack on [was: (no subject)] René Scharfe
2020-11-21  0:29   ` Jeff King [this message]
2020-11-21 21:54     ` Bug report: orphaned pack-objects after killing upload-pack on [ Junio C Hamano
2020-11-24  3:21       ` 唐宇奕
2020-11-24  9:11       ` Jeff King
2020-11-25 21:42         ` Junio C Hamano
2020-11-26  0:53           ` Jeff King
2020-11-26  1:04             ` Junio C Hamano
2020-11-26 20:04               ` René Scharfe
2020-11-27  4:17                 ` Jeff King
2020-11-27 20:43                   ` René Scharfe
2020-11-28  6:30                     ` Jeff King
2020-12-01 12:15                 ` Jeff King
2020-12-02 11:45                   ` René Scharfe
2020-12-02 22:14                     ` Junio C Hamano

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

  List information: http://vger.kernel.org/majordomo-info.html

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

  git send-email \
    --in-reply-to=20201121002921.GC353076@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=l.s.r@web.de \
    --cc=winglovet@gmail.com \
    /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

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git