# [PATCH] daemon: correctly handle soft accept() errors 14 messages from 2025-06-26 to 2025-07-01. Participants: Carlo Marcelo Arenas Belón, Kristoffer Haugsbakk, Phillip Wood, Junio C Hamano, Hridoy Ahmed. Thread: https://gitlist.dev/t/63700 ## Carlo Marcelo Arenas Belón, 2025-06-26 16:10 Subject: [PATCH] daemon: correctly handle soft accept() errors Message-ID: <20250626161038.85966-1-carenas@gmail.com> URL: https://gitlist.dev/e/20250626161038.85966-1-carenas%40gmail.com ``` Since df076bdbcc ([PATCH] GIT: Listen on IPv6 as well, if available., 2005-07-23), the original error checking was included in an inner loop unchanged, where its effect was different. Instead of retrying, after a EINTR during accept() in the listening socket, it will advance to the next one and try with that instead, leaving the client waiting for another round. Make sure that the loop doesn't advance and while at it, make sure that any possible completed childs get reaped earlier. To avoid an unlilely busy loop, fallback to the old behaviour after a couple of attempts. Signed-off-by: Carlo Marcelo Arenas Belón --- daemon.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/daemon.c b/daemon.c index d1be61fd57..8d02099a4e 100644 --- a/daemon.c +++ b/daemon.c @@ -1153,11 +1153,21 @@ static int service_loop(struct socketlist *socklist) #endif } ss; socklen_t sslen = sizeof(ss); - int incoming = accept(pfd[i].fd, &ss.sa, &sslen); + int incoming; + +redo: + incoming = accept(pfd[i].fd, &ss.sa, &sslen); if (incoming < 0) { + int retry = 2; + switch (errno) { - case EAGAIN: case EINTR: + if (--retry) { + check_dead_children(); + goto redo; + } + /* fallthrough */ + case EAGAIN: case ECONNABORTED: continue; default: -- 2.50.0.132.gb7f585ac00 ``` ## Kristoffer Haugsbakk, 2025-06-26 16:15 Subject: Re: [PATCH] daemon: correctly handle soft accept() errors Message-ID: <4e92bb11-1993-4cb2-be16-a972088f7cf3@app.fastmail.com> URL: https://gitlist.dev/e/4e92bb11-1993-4cb2-be16-a972088f7cf3%40app.fastmail.com In-Reply-To: <20250626161038.85966-1-carenas@gmail.com> ``` On Thu, Jun 26, 2025, at 18:10, Carlo Marcelo Arenas Belón wrote: > Since df076bdbcc ([PATCH] GIT: Listen on IPv6 as well, if available., > 2005-07-23), the original error checking was included in an inner loop > unchanged, where its effect was different. > > Instead of retrying, after a EINTR during accept() in the listening > socket, it will advance to the next one and try with that instead, > leaving the client waiting for another round. > > Make sure that the loop doesn't advance and while at it, make sure > that any possible completed childs get reaped earlier. To avoid an s/childs/children/ > unlilely busy loop, fallback to the old behaviour after a couple unlilely? > of attempts. > > Signed-off-by: Carlo Marcelo Arenas Belón ``` ## Carlo Marcelo Arenas Belón, 2025-06-26 17:21 Subject: [PATCH v2] daemon: correctly handle soft accept() errors in service_loop Message-ID: <20250626172159.87204-1-carenas@gmail.com> URL: https://gitlist.dev/e/20250626172159.87204-1-carenas%40gmail.com In-Reply-To: <20250626161038.85966-1-carenas@gmail.com> ``` Since df076bdbcc ([PATCH] GIT: Listen on IPv6 as well, if available., 2005-07-23), the original error checking was included in an inner loop unchanged, where its effect was different. Instead of retrying, after a EINTR during accept() in the listening socket, it will advance to the next one and try with that instead, leaving the client waiting for another round. Make sure that the loop doesn't advance and while at it, make sure that any possible completed children get reaped earlier. To avoid an unlikely busy loop, fallback to the old behaviour after a couple of attempts. Signed-off-by: Carlo Marcelo Arenas Belón --- daemon.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/daemon.c b/daemon.c index d1be61fd57..f113839781 100644 --- a/daemon.c +++ b/daemon.c @@ -1145,6 +1145,7 @@ static int service_loop(struct socketlist *socklist) for (size_t i = 0; i < socklist->nr; i++) { if (pfd[i].revents & POLLIN) { + int incoming; union { struct sockaddr sa; struct sockaddr_in sai; @@ -1153,11 +1154,19 @@ static int service_loop(struct socketlist *socklist) #endif } ss; socklen_t sslen = sizeof(ss); - int incoming = accept(pfd[i].fd, &ss.sa, &sslen); + int retry = 3; + + redo: + incoming = accept(pfd[i].fd, &ss.sa, &sslen); if (incoming < 0) { switch (errno) { - case EAGAIN: case EINTR: + if (--retry) { + check_dead_children(); + goto redo; + } + /* fallthrough */ + case EAGAIN: case ECONNABORTED: continue; default: -- 2.50.0.132.g195eec4876 ``` ## Phillip Wood, 2025-06-27 08:38 Subject: Re: [PATCH v2] daemon: correctly handle soft accept() errors in service_loop Message-ID: <08804dbe-56dd-4c0e-b36b-a82768b0aa29@gmail.com> URL: https://gitlist.dev/e/08804dbe-56dd-4c0e-b36b-a82768b0aa29%40gmail.com In-Reply-To: <20250626172159.87204-1-carenas@gmail.com> ``` Hi Carlo On 26/06/2025 18:21, Carlo Marcelo Arenas Belón wrote: > Since df076bdbcc ([PATCH] GIT: Listen on IPv6 as well, if available., > 2005-07-23), the original error checking was included in an inner loop > unchanged, where its effect was different. > > Instead of retrying, after a EINTR during accept() in the listening > socket, it will advance to the next one and try with that instead, > leaving the client waiting for another round. > > Make sure that the loop doesn't advance That makes sense > and while at it, make sure > that any possible completed children get reaped earlier. What's the rationale for that? It means we end up calling reap_dead_children() twice which sounds inefficient. If it is important then I'm also struggling to see how it fits in with the proposal to use SA_RESTART > To avoid an > unlikely busy loop, fallback to the old behaviour after a couple > of attempts. > > Signed-off-by: Carlo Marcelo Arenas Belón > --- > daemon.c | 13 +++++++++++-- > 1 file changed, 11 insertions(+), 2 deletions(-) > > diff --git a/daemon.c b/daemon.c > index d1be61fd57..f113839781 100644 > --- a/daemon.c > +++ b/daemon.c > @@ -1145,6 +1145,7 @@ static int service_loop(struct socketlist *socklist) > > for (size_t i = 0; i < socklist->nr; i++) { > if (pfd[i].revents & POLLIN) { > + int incoming; > union { > struct sockaddr sa; > struct sockaddr_in sai; > @@ -1153,11 +1154,19 @@ static int service_loop(struct socketlist *socklist) > #endif > } ss; > socklen_t sslen = sizeof(ss); > - int incoming = accept(pfd[i].fd, &ss.sa, &sslen); Why is the declaration of incoming moved but retry is declared here? Thanks Phillip > + int retry = 3; > + > + redo: > + incoming = accept(pfd[i].fd, &ss.sa, &sslen); > if (incoming < 0) { > switch (errno) { > - case EAGAIN: > case EINTR: > + if (--retry) { > + check_dead_children(); > + goto redo; > + } > + /* fallthrough */ > + case EAGAIN: > case ECONNABORTED: > continue; > default: ``` ## Carlo Marcelo Arenas Belón, 2025-06-27 19:05 Subject: Re: [PATCH v2] daemon: correctly handle soft accept() errors in service_loop Message-ID: URL: https://gitlist.dev/e/u4scxaxivz62fsljo7okkfdvcle3zdby6h2sdzd6ed5n6wi5xb%405ekxdycixwxe In-Reply-To: <08804dbe-56dd-4c0e-b36b-a82768b0aa29@gmail.com> ``` On Fri, Jun 27, 2025 at 09:38:47AM -0800, Phillip Wood wrote: > > On 26/06/2025 18:21, Carlo Marcelo Arenas Belón wrote: > > > > diff --git a/daemon.c b/daemon.c > > index d1be61fd57..f113839781 100644 > > --- a/daemon.c > > +++ b/daemon.c > > @@ -1145,6 +1145,7 @@ static int service_loop(struct socketlist *socklist) > > for (size_t i = 0; i < socklist->nr; i++) { > > if (pfd[i].revents & POLLIN) { > > + int incoming; > > union { > > struct sockaddr sa; > > struct sockaddr_in sai; > > @@ -1153,11 +1154,19 @@ static int service_loop(struct socketlist *socklist) > > #endif > > } ss; > > socklen_t sslen = sizeof(ss); > > - int incoming = accept(pfd[i].fd, &ss.sa, &sslen); > > Why is the declaration of incoming moved but retry is declared here? Separating the declaration and assignment for incoming is needed so we can insert a label for goto; moving it up just removes distractions so the rest of the logic is clearly in view. Obviously that includes the definition and assignment for retry. How would you suggest to arrange this better? Carlo ``` ## Junio C Hamano, 2025-06-27 20:19 Subject: Re: [PATCH v2] daemon: correctly handle soft accept() errors in service_loop Message-ID: URL: https://gitlist.dev/e/xmqq34bl7xa1.fsf%40gitster.g In-Reply-To: ``` Carlo Marcelo Arenas Belón writes: > On Fri, Jun 27, 2025 at 09:38:47AM -0800, Phillip Wood wrote: >> >> On 26/06/2025 18:21, Carlo Marcelo Arenas Belón wrote: >> > >> > diff --git a/daemon.c b/daemon.c >> > index d1be61fd57..f113839781 100644 >> > --- a/daemon.c >> > +++ b/daemon.c >> > @@ -1145,6 +1145,7 @@ static int service_loop(struct socketlist *socklist) >> > for (size_t i = 0; i < socklist->nr; i++) { >> > if (pfd[i].revents & POLLIN) { >> > + int incoming; >> > union { >> > struct sockaddr sa; >> > struct sockaddr_in sai; >> > @@ -1153,11 +1154,19 @@ static int service_loop(struct socketlist *socklist) >> > #endif >> > } ss; >> > socklen_t sslen = sizeof(ss); >> > - int incoming = accept(pfd[i].fd, &ss.sa, &sslen); >> >> Why is the declaration of incoming moved but retry is declared here? > > Separating the declaration and assignment for incoming is needed so we can > insert a label for goto; moving it up just removes distractions so the rest > of the logic is clearly in view. > > Obviously that includes the definition and assignment for retry. > > How would you suggest to arrange this better? I think what Phillip meant was more like this, perhaps. socklen_t sslen = sizeof(ss); - int incoming = accept(pfd[i].fd, &ss.sa, &sslen); + int incoming; + int retry = 3; + + incoming = accept(pfd[i].fd, &ss.sa, &sslen); if (incoming < 0) { ... ``` ## Carlo Marcelo Arenas Belón, 2025-06-27 23:05 Subject: Re: [PATCH v2] daemon: correctly handle soft accept() errors in service_loop Message-ID: URL: https://gitlist.dev/e/vgailqqh3bcip3gxtdffoo4ey7xjso4xerewxncy22shrzn4k2%4025hst4sfgxq4 In-Reply-To: ``` On Fri, Jun 27, 2025 at 01:19:18PM -0800, Junio C Hamano wrote: > Carlo Marcelo Arenas Belón writes: > > > On Fri, Jun 27, 2025 at 09:38:47AM -0800, Phillip Wood wrote: > >> > >> On 26/06/2025 18:21, Carlo Marcelo Arenas Belón wrote: > >> > > >> > diff --git a/daemon.c b/daemon.c > >> > index d1be61fd57..f113839781 100644 > >> > --- a/daemon.c > >> > +++ b/daemon.c > >> > @@ -1145,6 +1145,7 @@ static int service_loop(struct socketlist *socklist) > >> > for (size_t i = 0; i < socklist->nr; i++) { > >> > if (pfd[i].revents & POLLIN) { > >> > + int incoming; > >> > union { > >> > struct sockaddr sa; > >> > struct sockaddr_in sai; > >> > @@ -1153,11 +1154,19 @@ static int service_loop(struct socketlist *socklist) > >> > #endif > >> > } ss; > >> > socklen_t sslen = sizeof(ss); > >> > - int incoming = accept(pfd[i].fd, &ss.sa, &sslen); > >> > >> Why is the declaration of incoming moved but retry is declared here? > > > > Separating the declaration and assignment for incoming is needed so we can > > insert a label for goto; moving it up just removes distractions so the rest > > of the logic is clearly in view. > > > > Obviously that includes the definition and assignment for retry. > > > > How would you suggest to arrange this better? > > I think what Phillip meant was more like this, perhaps. > > socklen_t sslen = sizeof(ss); > - int incoming = accept(pfd[i].fd, &ss.sa, &sslen); > + int incoming; > + int retry = 3; > + > + incoming = accept(pfd[i].fd, &ss.sa, &sslen); > if (incoming < 0) { > ... That seems unnecessarily restrictive just to minimize churn and leaves the deflaration of incoming strangely sitting in between two assignments, which while it doesn't trigger -Wdeclaration-after-statement seems to go against its spirit. Will include in a v3 with all other suggestions, but frankly think that the original was overall cleaner. Carlo ``` ## Carlo Marcelo Arenas Belón, 2025-06-27 23:14 Subject: [PATCH v3] daemon: correctly handle soft accept() errors in service_loop Message-ID: <20250627231404.27100-1-carenas@gmail.com> URL: https://gitlist.dev/e/20250627231404.27100-1-carenas%40gmail.com In-Reply-To: <20250626172159.87204-1-carenas@gmail.com> ``` Since df076bdbcc ([PATCH] GIT: Listen on IPv6 as well, if available., 2005-07-23), the original error checking was included in an inner loop unchanged, where its effect was different. Instead of retrying, after a EINTR during accept() in the listening socket, it will advance to the next one and try with that instead, leaving the client waiting for another round. Make sure to retry with the same listener socket that failed originally. To avoid an unlikely busy loop, fallback to the old behaviour after a couple of attempts. Signed-off-by: Carlo Marcelo Arenas Belón --- daemon.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/daemon.c b/daemon.c index d1be61fd57..9ac9efa17c 100644 --- a/daemon.c +++ b/daemon.c @@ -1153,11 +1153,19 @@ static int service_loop(struct socketlist *socklist) #endif } ss; socklen_t sslen = sizeof(ss); - int incoming = accept(pfd[i].fd, &ss.sa, &sslen); + int incoming; + int retry = 3; + + redo: + incoming = accept(pfd[i].fd, &ss.sa, &sslen); if (incoming < 0) { switch (errno) { - case EAGAIN: case EINTR: + if (--retry) + goto redo; + + /* fallthrough */ + case EAGAIN: case ECONNABORTED: continue; default: -- 2.50.0.132.g195eec4876.dirty ``` ## Hridoy Ahmed, 2025-06-27 23:25 Subject: Re: [PATCH v2] daemon: correctly handle soft accept() errors in service_loop Message-ID: <061ECE45-21BA-4216-8F3A-61D5A8314706@gmail.com> URL: https://gitlist.dev/e/061ECE45-21BA-4216-8F3A-61D5A8314706%40gmail.com In-Reply-To: ``` Hridoy Ahmed > On 28 Jun 2025, at 2:06 AM, Carlo Marcelo Arenas Belón wrote: > > On Fri, Jun 27, 2025 at 01:19:18PM -0800, Junio C Hamano wrote: >> Carlo Marcelo Arenas Belón writes: >> >>> On Fri, Jun 27, 2025 at 09:38:47AM -0800, Phillip Wood wrote: >>>> >>>>> On 26/06/2025 18:21, Carlo Marcelo Arenas Belón wrote: >>>>>> >>>>>> diff --git a/daemon.c b/daemon.c >>>>>> index d1be61fd57..f113839781 100644 >>>>>> --- a/daemon.c >>>>>> +++ b/daemon.c >>>>>> @@ -1145,6 +1145,7 @@ static int service_loop(struct socketlist *socklist) >>>>>> for (size_t i = 0; i < socklist->nr; i++) { >>>>>> if (pfd[i].revents & POLLIN) { >>>>>> + int incoming; >>>>>> union { >>>>>> struct sockaddr sa; >>>>>> struct sockaddr_in sai; >>>>>> @@ -1153,11 +1154,19 @@ static int service_loop(struct socketlist *socklist) >>>>>> #endif >>>>>> } ss; >>>>>> socklen_t sslen = sizeof(ss); >>>>>> - int incoming = accept(pfd[i].fd, &ss.sa, &sslen); >>>>> >>>>> Why is the declaration of incoming moved but retry is declared here? >>> >>> Separating the declaration and assignment for incoming is needed so we can >>> insert a label for goto; moving it up just removes distractions so the rest >>> of the logic is clearly in view. >>> >>> Obviously that includes the definition and assignment for retry. >>> >>> How would you suggest to arrange this better? >> >> I think what Phillip meant was more like this, perhaps. >> >> socklen_t sslen = sizeof(ss); >> - int incoming = accept(pfd[i].fd, &ss.sa, &sslen); >> + int incoming; >> + int retry = 3; >> + >> + incoming = accept(pfd[i].fd, &ss.sa, &sslen); >> if (incoming < 0) { >> ... > > That seems unnecessarily restrictive just to minimize churn and leaves the > deflaration of incoming strangely sitting in between two assignments, which > while it doesn't trigger -Wdeclaration-after-statement seems to go against > its spirit. > > Will include in a v3 with all other suggestions, but frankly think that the > original was overall cleaner. > > Carlo > ``` ## Junio C Hamano, 2025-06-27 23:53 Subject: Re: [PATCH v2] daemon: correctly handle soft accept() errors in service_loop Message-ID: URL: https://gitlist.dev/e/xmqqy0tc68s4.fsf%40gitster.g In-Reply-To: ``` Carlo Marcelo Arenas Belón writes: >> socklen_t sslen = sizeof(ss); >> - int incoming = accept(pfd[i].fd, &ss.sa, &sslen); >> + int incoming; >> + int retry = 3; >> + >> + incoming = accept(pfd[i].fd, &ss.sa, &sslen); >> if (incoming < 0) { >> ... > > That seems unnecessarily restrictive just to minimize churn and leaves the > deflaration of incoming strangely sitting in between two assignments, which > while it doesn't trigger -Wdeclaration-after-statement seems to go against > its spirit. Hmph, I am not Phillip, but my take on it is that incoming and retry are fairly closely related variables in this loop, and better grouped together? I also find it a bit ugly to hardcode "3" here like this, but perhaps I am overthinking about it. ``` ## Phillip Wood, 2025-06-30 08:59 Subject: Re: [PATCH v2] daemon: correctly handle soft accept() errors in service_loop Message-ID: URL: https://gitlist.dev/e/abf3aabb-8e46-4324-9e35-634cdaf110fb%40gmail.com In-Reply-To: ``` On 28/06/2025 00:53, Junio C Hamano wrote: > Carlo Marcelo Arenas Belón writes: > >> That seems unnecessarily restrictive just to minimize churn and leaves the >> deflaration of incoming strangely sitting in between two assignments, which >> while it doesn't trigger -Wdeclaration-after-statement seems to go against >> its spirit. > > Hmph, I am not Phillip, but my take on it is that incoming and retry > are fairly closely related variables in this loop, and better > grouped together? That was my thinking too Thanks Phillip ``` ## Phillip Wood, 2025-06-30 09:00 Subject: Re: [PATCH v3] daemon: correctly handle soft accept() errors in service_loop Message-ID: <0d507273-8b8c-42d9-a14f-27a5da0dac27@gmail.com> URL: https://gitlist.dev/e/0d507273-8b8c-42d9-a14f-27a5da0dac27%40gmail.com In-Reply-To: <20250627231404.27100-1-carenas@gmail.com> ``` Hi Carlo This looks good Thanks Phillip On 28/06/2025 00:14, Carlo Marcelo Arenas Belón wrote: > Since df076bdbcc ([PATCH] GIT: Listen on IPv6 as well, if available., > 2005-07-23), the original error checking was included in an inner loop > unchanged, where its effect was different. > > Instead of retrying, after a EINTR during accept() in the listening > socket, it will advance to the next one and try with that instead, > leaving the client waiting for another round. > > Make sure to retry with the same listener socket that failed originally. > > To avoid an unlikely busy loop, fallback to the old behaviour after a > couple of attempts. > > Signed-off-by: Carlo Marcelo Arenas Belón > --- > daemon.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/daemon.c b/daemon.c > index d1be61fd57..9ac9efa17c 100644 > --- a/daemon.c > +++ b/daemon.c > @@ -1153,11 +1153,19 @@ static int service_loop(struct socketlist *socklist) > #endif > } ss; > socklen_t sslen = sizeof(ss); > - int incoming = accept(pfd[i].fd, &ss.sa, &sslen); > + int incoming; > + int retry = 3; > + > + redo: > + incoming = accept(pfd[i].fd, &ss.sa, &sslen); > if (incoming < 0) { > switch (errno) { > - case EAGAIN: > case EINTR: > + if (--retry) > + goto redo; > + > + /* fallthrough */ > + case EAGAIN: > case ECONNABORTED: > continue; > default: ``` ## Junio C Hamano, 2025-06-30 15:33 Subject: Re: [PATCH v3] daemon: correctly handle soft accept() errors in service_loop Message-ID: URL: https://gitlist.dev/e/xmqqv7od452s.fsf%40gitster.g In-Reply-To: <0d507273-8b8c-42d9-a14f-27a5da0dac27@gmail.com> ``` Phillip Wood writes: > Hi Carlo > > This looks good > > Thanks > > Phillip Thanks, both of you. Shall we mark the topic for 'next', then? > On 28/06/2025 00:14, Carlo Marcelo Arenas Belón wrote: >> Since df076bdbcc ([PATCH] GIT: Listen on IPv6 as well, if available., >> 2005-07-23), the original error checking was included in an inner loop >> unchanged, where its effect was different. >> Instead of retrying, after a EINTR during accept() in the listening >> socket, it will advance to the next one and try with that instead, >> leaving the client waiting for another round. >> Make sure to retry with the same listener socket that failed >> originally. >> To avoid an unlikely busy loop, fallback to the old behaviour after >> a >> couple of attempts. >> Signed-off-by: Carlo Marcelo Arenas Belón >> --- >> daemon.c | 12 ++++++++++-- >> 1 file changed, 10 insertions(+), 2 deletions(-) >> diff --git a/daemon.c b/daemon.c >> index d1be61fd57..9ac9efa17c 100644 >> --- a/daemon.c >> +++ b/daemon.c >> @@ -1153,11 +1153,19 @@ static int service_loop(struct socketlist *socklist) >> #endif >> } ss; >> socklen_t sslen = sizeof(ss); >> - int incoming = accept(pfd[i].fd, &ss.sa, &sslen); >> + int incoming; >> + int retry = 3; >> + >> + redo: >> + incoming = accept(pfd[i].fd, &ss.sa, &sslen); >> if (incoming < 0) { >> switch (errno) { >> - case EAGAIN: >> case EINTR: >> + if (--retry) >> + goto redo; >> + >> + /* fallthrough */ >> + case EAGAIN: >> case ECONNABORTED: >> continue; >> default: ``` ## Phillip Wood, 2025-07-01 19:33 Subject: Re: [PATCH v3] daemon: correctly handle soft accept() errors in service_loop Message-ID: <628e4050-912e-491d-b55b-a054623db32a@gmail.com> URL: https://gitlist.dev/e/628e4050-912e-491d-b55b-a054623db32a%40gmail.com In-Reply-To: ``` On 30/06/2025 16:33, Junio C Hamano wrote: > > Thanks, both of you. Shall we mark the topic for 'next', then? Yes, I think so Thanks Phillip ```