From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: pve-devel@lists.proxmox.com
Subject: [pve-devel] [PATCH http-server] fix #4816: do not disconnect twice if client sends no data
Date: Wed, 4 Dec 2024 10:52:34 +0100 [thread overview]
Message-ID: <20241204095234.291884-1-f.gruenbichler@proxmox.com> (raw)
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
reply other threads:[~2024-12-04 9:53 UTC|newest]
Thread overview: [no followups] expand[flat|nested] mbox.gz Atom feed
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
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20241204095234.291884-1-f.gruenbichler@proxmox.com \
--to=f.gruenbichler@proxmox.com \
--cc=pve-devel@lists.proxmox.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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox