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: "Junio C Hamano" <gitster@pobox.com>, 唐宇奕 <winglovet@gmail.com>,
	git@vger.kernel.org
Subject: Re: Bug report: orphaned pack-objects after killing upload-pack on [
Date: Thu, 26 Nov 2020 23:17:43 -0500
Message-ID: <X8B9589XMlCQEltA@coredump.intra.peff.net> (raw)
In-Reply-To: <bd2c4577-4c8c-851c-6045-ba4b306ca612@web.de>

On Thu, Nov 26, 2020 at 09:04:35PM +0100, René Scharfe wrote:

> Before I could submit that one (or something similar) formally, I'd need
> to understand what's happening here a lot better and witness the effect
> of the patch.
> 
> I understand that the main benefit of stopping the child upon
> termination of the parent is to avoid using CPU cycles on a heavy task
> whose results will just go to waste.  But wouldn't the orphaned child
> then become a zombie?  Init would reap it eventually, but are there
> perhaps init-less deployments (containerized daemon?) where such
> zombies could pile up?

I think an init-less deployment like that is already broken. If we
encounter any error at all in upload-pack we may quit without reaping
all of our children. And this could never be protected against entirely;
we could be killed by SIGSEGV, SIGKILL, etc.

My understanding is container deployments often have a tiny pid-1 init
that takes care of zombie processes like this (but it's not something
I've dealt with much myself).

> For a test, winning the race condition should be easy if we cheat by
> letting the child loop forever.  But I struggle even with the most
> basic task: Making upload-pack invoked by clone call pack-objects.
> (Feeling a bit silly.)

Here's an easy reproduction. On a clone of something large-ish (by
number of objects) like linux.git:

  - make sure you don't have bitmaps on (since they make the enumerating
    phase go quickly). For linux.git it takes ~30s or so to walk the
    whole graph on my machine.

  - run "git clone --no-local -q . dst"; the "-q" is important because
    if pack-objects is writing progress to upload-pack (to get
    multiplexed over the sideband to the client), then it will notice
    pretty quickly the failure to write to stderr

  - kill just upload-pack with "pkill git-upload-pack" or whatever you
    like

  - run "ps au | grep pack-objects" (or just "top") to see pack-objects
    chugging on 100% CPU (and consuming 1GB+ of RAM)

With the patch adding clean_on_exit, that last step turns up nothing.

Now the situation above is probably pretty rare. Nobody is usually going
to kill upload-pack specifically. The more common case is when
upload-pack realizes that the client (or the network) has gone away,
because it tries to write and finds the connection gone. But what is it
writing? Most of the time it's stuff from pack-objects! So in the normal
case, pack-objects is continually writing either data or progress
reports, so it would notice for its next write.

But again, a client asking for no progress is a problem. upload-pack
will be sending keepalives every 5s or so, so it will notice client
death then. But pack-objects will keep running, not generating any
output until it starts spewing the pack.

So you could probably make the scenario above a bit more realistic by
killing the parent git-clone process. But don't use ^C; that will send
SIGINT to all of the processes. Simulate a network failure by killing
the "git clone" process specifically. This shows the same problem, and
the same improvement after the patch (though remember it may take up to
5 seconds for upload-pack to send a keepalive and notice the problem).

-Peff

  reply	other threads:[~2020-11-27  4:19 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
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 [this message]
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=X8B9589XMlCQEltA@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --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