[PATCH] daemon: correctly handle soft accept() errors

[PATCH] daemon: correctly handle soft accept() errors

[Carlo Marcelo Arenas Belón]

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 <carenas@gmail.com>
---
 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

Re: [PATCH] daemon: correctly handle soft accept() errors

[Kristoffer Haugsbakk]

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 <carenas@gmail.com>

[PATCH v2] daemon: correctly handle soft accept() errors in service_loop

[Carlo Marcelo Arenas Belón]

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 <carenas@gmail.com>
---
 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

Re: [PATCH v2] daemon: correctly handle soft accept() errors in service_loop

[Phillip Wood]

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 <carenas@gmail.com>
> ---
>   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:

Re: [PATCH v2] daemon: correctly handle soft accept() errors in service_loop

[Carlo Marcelo Arenas Belón]

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

Re: [PATCH v2] daemon: correctly handle soft accept() errors in service_loop

[Junio C Hamano]

Carlo Marcelo Arenas Belón <carenas@gmail.com> 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) {
			...

Re: [PATCH v2] daemon: correctly handle soft accept() errors in service_loop

[Carlo Marcelo Arenas Belón]

On Fri, Jun 27, 2025 at 01:19:18PM -0800, Junio C Hamano wrote:
> Carlo Marcelo Arenas Belón <carenas@gmail.com> 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

[PATCH v3] daemon: correctly handle soft accept() errors in service_loop

[Carlo Marcelo Arenas Belón]

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 <carenas@gmail.com>
---
 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

Re: [PATCH v2] daemon: correctly handle soft accept() errors in service_loop

[Hridoy Ahmed]

Hridoy Ahmed

> On 28 Jun 2025, at 2:06 AM, Carlo Marcelo Arenas Belón <carenas@gmail.com> wrote:
> 
> On Fri, Jun 27, 2025 at 01:19:18PM -0800, Junio C Hamano wrote:
>> Carlo Marcelo Arenas Belón <carenas@gmail.com> 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
> 

Re: [PATCH v2] daemon: correctly handle soft accept() errors in service_loop

[Junio C Hamano]

Carlo Marcelo Arenas Belón <carenas@gmail.com> 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.

Re: [PATCH v2] daemon: correctly handle soft accept() errors in service_loop

[Phillip Wood]

On 28/06/2025 00:53, Junio C Hamano wrote:
> Carlo Marcelo Arenas Belón <carenas@gmail.com> 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

Re: [PATCH v3] daemon: correctly handle soft accept() errors in service_loop

[Phillip Wood]

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 <carenas@gmail.com>
> ---
>   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:

Re: [PATCH v3] daemon: correctly handle soft accept() errors in service_loop

[Junio C Hamano]

Phillip Wood <phillip.wood123@gmail.com> 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 <carenas@gmail.com>
>> ---
>>   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:

Re: [PATCH v3] daemon: correctly handle soft accept() errors in service_loop

[Phillip Wood]

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