From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on dcvr.yhbt.net X-Spam-Level: X-Spam-Status: No, score=-3.8 required=3.0 tests=AWL,BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_PASS, SPF_PASS shortcircuit=no autolearn=ham autolearn_force=no version=3.4.2 Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by dcvr.yhbt.net (Postfix) with ESMTP id 9C1E01F86C for ; Sat, 28 Nov 2020 22:55:24 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1731374AbgK1Vtv (ORCPT ); Sat, 28 Nov 2020 16:49:51 -0500 Received: from cloud.peff.net ([104.130.231.41]:44948 "EHLO cloud.peff.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2387570AbgK1T5b (ORCPT ); Sat, 28 Nov 2020 14:57:31 -0500 Received: (qmail 26275 invoked by uid 109); 28 Nov 2020 06:30:09 -0000 Received: from Unknown (HELO peff.net) (10.0.1.2) by cloud.peff.net (qpsmtpd/0.94) with ESMTP; Sat, 28 Nov 2020 06:30:09 +0000 Authentication-Results: cloud.peff.net; auth=none Received: (qmail 30667 invoked by uid 111); 28 Nov 2020 06:30:07 -0000 Received: from coredump.intra.peff.net (HELO sigill.intra.peff.net) (10.0.0.2) by peff.net (qpsmtpd/0.94) with (TLS_AES_256_GCM_SHA384 encrypted) ESMTPS; Sat, 28 Nov 2020 01:30:07 -0500 Authentication-Results: peff.net; auth=none Date: Sat, 28 Nov 2020 01:30:07 -0500 From: Jeff King To: =?utf-8?B?UmVuw6k=?= Scharfe Cc: Junio C Hamano , =?utf-8?B?5ZSQ5a6H5aWV?= , git@vger.kernel.org Subject: Re: Bug report: orphaned pack-objects after killing upload-pack on [ Message-ID: References: <20201121002921.GC353076@coredump.intra.peff.net> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: git@vger.kernel.org On Fri, Nov 27, 2020 at 09:43:06PM +0100, René Scharfe wrote: > > [...zombie processes...] > OK, so overall the situation sounds a bit messy to me and perhaps > there's room for improvement, but I agree now that we can leave the > specialists (init, tini) to deal with our zombies. Keep in mind we are not changing anything here, either. clean_on_exit is kicking in when we already would be exiting without calling wait(). We could _also_ instruct run-command to wait, but nobody seems to be complaining about it. > With the debug patch above and GIT_DEBUG_ABANDON_CHILD=git-upload-pack I > need the following patch get rid of the spawned process: I don't think that is an interesting case, though. We've been discussing the spawn between upload-pack and its child pack-objects, but here you're running a bogus infinite loop instead of upload-pack. It will spin forever, because the client is expecting it to say something. In a normal setup, a severing of the connection between the client and upload-pack will be noticed quickly, because each one is always either trying to read from or write to the other (the exception is while pack-objects is processing without progress meters, but there we send a keep-alive every 5 seconds). If one of them were to spin in a true infinite loop due to a bug (but not break the connection), we would wait forever. But the solution there is a timeout on inactivity. So... > --- >8 --- > > diff --git a/connect.c b/connect.c > index 8b8f56cf6d..e1b1b73ef5 100644 > --- a/connect.c > +++ b/connect.c > @@ -1369,6 +1369,7 @@ struct child_process *git_connect(int fd[2], const char *url, > > conn->use_shell = 1; > conn->in = conn->out = -1; > + conn->clean_on_exit = 1; > if (protocol == PROTO_SSH) { > char *ssh_host = hostandport; > const char *port = NULL; > > --- 8< --- I don't think there's much point here. If the client side dies, it will close the socket, and upload-pack would notice anyway. It's only your fake "spin without doing any I/O" patch that misbehaves. Moreover, this wouldn't do _anything_ in many cases, because upload-pack is going to be on the far side of a network socket. So really you'd be killing ssh here for those cases (which then likewise would close the socket, but this isn't necessary because ssh will also exit when it sees us close _our_ end). And it would do nothing at all for git-over-http, git://, etc. > So is there a downside to clean_on_exit? It doesn't make sense when we > start browsers or pagers, but for hooks and helpers (which are probably > the majority of started processes) cascading program termination makes > sense, no? In general, no, I suspect clean_on_exit wouldn't be hurting anything if we used it more. But generally we get a natural cascade of termination because exiting processes close the input and output pipes going to the other sub-processes. -Peff