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: Sat, 28 Nov 2020 01:30:07 -0500 Message-ID: <X8Hub2OeFy3V6MhV@coredump.intra.peff.net> (raw) In-Reply-To: <d0fcfa7f-9835-a9e6-c4a4-af4de177ff8c@web.de> 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
next prev parent reply other threads:[~2020-11-28 22:55 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 2020-11-27 20:43 ` René Scharfe 2020-11-28 6:30 ` Jeff King [this message] 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=X8Hub2OeFy3V6MhV@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