threads / patch / 63700

patchdaemon: correctly handle soft accept() errors

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

## tl;dr

14 messages between Jun 26, 2025 and Jul 1, 2025. Diffs are folded; open one to read it.

replies: 13people: 5as markdown or json

Carlo Marcelo Arenas Belón· Jun 26, 2025, 16:10 UTC · lore

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(-)
Show changes to daemon.c +12 −2
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· Jun 26, 2025, 16:15 UTC · re: Carlo Marcelo Arenas Belón · lore

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

On Thu, Jun 26, 2025, at 18:10, Carlo Marcelo Arenas Belón wrote:
Show 10 quoted lines
> 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>
Carlo Marcelo Arenas Belón· Jun 26, 2025, 17:21 UTC · re: Carlo Marcelo Arenas Belón · lore

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

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(-)
Show changes to daemon.c +11 −2
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· Jun 27, 2025, 08:38 UTC · re: Carlo Marcelo Arenas Belón · lore

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

Hi Carlo
On 26/06/2025 18:21, Carlo Marcelo Arenas Belón wrote:
Show 9 quoted lines
> 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

Show 26 quoted lines
> 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
Show 17 quoted lines
> +				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· Jun 27, 2025, 19:05 UTC · re: Phillip Wood · lore

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

On Fri, Jun 27, 2025 at 09:38:47AM -0800, Phillip Wood wrote:
Show 21 quoted lines
> 
> 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· Jun 27, 2025, 20:19 UTC · re: Carlo Marcelo Arenas Belón · lore

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

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:
Show 30 quoted lines
> 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· Jun 27, 2025, 23:05 UTC · re: Junio C Hamano · lore

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

On Fri, Jun 27, 2025 at 01:19:18PM -0800, Junio C Hamano wrote:
Show 43 quoted lines
> 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
Hridoy Ahmed· Jun 27, 2025, 23:25 UTC · re: Carlo Marcelo Arenas Belón · lore

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

Hridoy Ahmed
Show 57 quoted lines
> 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
>
Junio C Hamano· Jun 27, 2025, 23:53 UTC · re: Carlo Marcelo Arenas Belón · lore

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

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:
Show 13 quoted lines
>> 		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· Jun 30, 2025, 08:59 UTC · re: Junio C Hamano · lore

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

On 28/06/2025 00:53, Junio C Hamano wrote:
Show 10 quoted lines
> 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
Carlo Marcelo Arenas Belón· Jun 27, 2025, 23:14 UTC · re: Carlo Marcelo Arenas Belón · lore

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

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(-)
Show changes to daemon.c +10 −2
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
Phillip Wood· Jun 30, 2025, 09:00 UTC · re: Carlo Marcelo Arenas Belón · lore

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

Hi Carlo
This looks good
Thanks
Phillip
On 28/06/2025 00:14, Carlo Marcelo Arenas Belón wrote:
Show 44 quoted lines
> 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:
Junio C Hamano· Jun 30, 2025, 15:33 UTC · re: Phillip Wood · lore

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

Phillip Wood <phillip.wood123@gmail.com> writes:
Show 7 quoted lines
> Hi Carlo
>
> This looks good
>
> Thanks
>
> Phillip
Thanks, both of you.  Shall we mark the topic for 'next', then?
Show 42 quoted lines
> 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:
Phillip Wood· Jul 1, 2025, 19:33 UTC · re: Junio C Hamano · lore

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

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

← back to recent threads