* [pve-devel] [PATCH http-server] fix #4816: do not disconnect twice if client sends no data
@ 2024-12-04 9:52 Fabian Grünbichler
2025-01-28 14:30 ` [pve-devel] applied: " Thomas Lamprecht
0 siblings, 1 reply; 2+ messages in thread
From: Fabian Grünbichler @ 2024-12-04 9:52 UTC (permalink / raw)
To: pve-devel
client_do_disconnect expects to be called exactly once per connection, since it
takes care of closing and unsetting the handle corresponding to the connection.
to find bugs in our connection handling, it will log "detected empty handle" if
it is called for a request/connection that no longer has a handle.
the edge case of opening a connection without sending any data leads to the
error callback being called twice:
Dec 04 09:37:02 xxx pveproxy[175235]: err (): Connection timed out
this is the (5 second) timeout triggering
Dec 04 09:37:02 xxx pveproxy[175235]: err (1): Broken pipe
this is AnyEvent trying to drain the buffer while the connection is already
closed
as soon as a single byte of traffic is sent, only the timeout will trigger.
there is no guarantee that the on_error callback is only called once (in fact,
it's possible to return from it for non-fatal errors and continue processing
the connection).
if there are further reports of empty handles with this in place, other
on_error callbacks might need similar logic - but it should only be added if
the triggering conditions are clear and deemed safe. the additional logging is
only cosmetic after all, but might point out an actual issue in our connection
handling code.
Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
src/PVE/APIServer/AnyEvent.pm | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.pm
index 24209a1..6c165c8 100644
--- a/src/PVE/APIServer/AnyEvent.pm
+++ b/src/PVE/APIServer/AnyEvent.pm
@@ -1867,7 +1867,13 @@ sub accept_connections {
my ($hdl, $fatal, $message) = @_;
eval {
$self->log_aborted_request($reqstate, $message);
- $self->client_do_disconnect($reqstate);
+ # this error callback can be called twice for the same
+ # connection/handle if the timeout is reached before
+ # any data has been received, avoid misleading errors
+ if (!$reqstate->{disconnected}) {
+ $self->client_do_disconnect($reqstate);
+ $reqstate->{disconnected} = 1;
+ }
};
if (my $err = $@) { syslog('err', "$err"); }
},
--
2.39.5
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 2+ messages in thread
* [pve-devel] applied: [PATCH http-server] fix #4816: do not disconnect twice if client sends no data
2024-12-04 9:52 [pve-devel] [PATCH http-server] fix #4816: do not disconnect twice if client sends no data Fabian Grünbichler
@ 2025-01-28 14:30 ` Thomas Lamprecht
0 siblings, 0 replies; 2+ messages in thread
From: Thomas Lamprecht @ 2025-01-28 14:30 UTC (permalink / raw)
To: Proxmox VE development discussion, Fabian Grünbichler
Am 04.12.24 um 10:52 schrieb Fabian Grünbichler:
> client_do_disconnect expects to be called exactly once per connection, since it
> takes care of closing and unsetting the handle corresponding to the connection.
> to find bugs in our connection handling, it will log "detected empty handle" if
> it is called for a request/connection that no longer has a handle.
>
> the edge case of opening a connection without sending any data leads to the
> error callback being called twice:
>
> Dec 04 09:37:02 xxx pveproxy[175235]: err (): Connection timed out
>
> this is the (5 second) timeout triggering
>
> Dec 04 09:37:02 xxx pveproxy[175235]: err (1): Broken pipe
>
> this is AnyEvent trying to drain the buffer while the connection is already
> closed
>
> as soon as a single byte of traffic is sent, only the timeout will trigger.
>
> there is no guarantee that the on_error callback is only called once (in fact,
> it's possible to return from it for non-fatal errors and continue processing
> the connection).
>
> if there are further reports of empty handles with this in place, other
> on_error callbacks might need similar logic - but it should only be added if
> the triggering conditions are clear and deemed safe. the additional logging is
> only cosmetic after all, but might point out an actual issue in our connection
> handling code.
>
> Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
> ---
> src/PVE/APIServer/AnyEvent.pm | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
>
applied, thanks!
_______________________________________________
pve-devel mailing list
pve-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2025-01-28 14:30 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2024-12-04 9:52 [pve-devel] [PATCH http-server] fix #4816: do not disconnect twice if client sends no data Fabian Grünbichler
2025-01-28 14:30 ` [pve-devel] applied: " Thomas Lamprecht
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox