From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id BDD3A7D2D1 for ; Mon, 8 Nov 2021 16:46:02 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id A778D28754 for ; Mon, 8 Nov 2021 16:45:32 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 3650028747 for ; Mon, 8 Nov 2021 16:45:31 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 0280B457FB for ; Mon, 8 Nov 2021 16:45:31 +0100 (CET) Date: Mon, 08 Nov 2021 16:45:19 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Fabian Ebner , pve-devel@lists.proxmox.com References: <20211105130359.40803-1-f.gruenbichler@proxmox.com> <20211105130359.40803-13-f.gruenbichler@proxmox.com> <9df61960-edb3-1dc2-5e75-993817e9f024@proxmox.com> In-Reply-To: <9df61960-edb3-1dc2-5e75-993817e9f024@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1636385624.81iw4jegre.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.295 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Subject: Re: [pve-devel] [PATCH http-server 1/1] webproxy: handle unflushed write buffer X-BeenThere: pve-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox VE development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 08 Nov 2021 15:46:02 -0000 On November 8, 2021 3:15 pm, Fabian Ebner wrote: > Am 05.11.21 um 14:03 schrieb Fabian Gr=C3=BCnbichler: >> for proxied requests, we usually tear down the proxy connection >> immediately when closing the source connection. this is not the correct >> course of action for bulk one-way data streams that are proxied, where >> the source connection might be closed, but the proxy connection might >> still have data in the write buffer that needs to be written out. >>=20 >> push_shutdown already handles this case (closing the socket/FH after it >> has been fully drained). >>=20 >> one example for such a proxied data stream is the 'migrate' data for a >> remote migration, which gets proxied over a websocket connection. >> terminating the proxied connection early makes the target VM crash for >> obvious reasons. >>=20 >> Signed-off-by: Fabian Gr=C3=BCnbichler >> --- >> src/PVE/APIServer/AnyEvent.pm | 10 ++++++---- >> 1 file changed, 6 insertions(+), 4 deletions(-) >>=20 >> diff --git a/src/PVE/APIServer/AnyEvent.pm b/src/PVE/APIServer/AnyEvent.= pm >> index 86d0e2e..ecf771f 100644 >> --- a/src/PVE/APIServer/AnyEvent.pm >> +++ b/src/PVE/APIServer/AnyEvent.pm >> @@ -144,7 +144,8 @@ sub client_do_disconnect { >> }; >> =20 >> if (my $proxyhdl =3D delete $reqstate->{proxyhdl}) { >> - &$shutdown_hdl($proxyhdl); >> + &$shutdown_hdl($proxyhdl) >> + if !$proxyhdl->{block_disconnect}; >=20 > Style nit: fits in one line ;) >=20 > I'm not familiar with the code, so I'll just ask: can this be reached=20 > without going through the code below first, i.e. can it happen that=20 > block_disconnect is not set, but length $proxyhdl->{wbuf} > 0? Or is it=20 > not important in other cases (if there are any)? in theory, yes. in practice, not likely with anything that matters ;) we have - spiceproxy, if the client closes the connection we likely=20 don't care about the last few bytes of input being dropped - proxied API requests with response_stream - never gets written to - WS proxy (fixed in this patch ;) technically also used like=20 spiceproxy) but I have to admit I did think about doing this always and relying on=20 timeouts to clear the proxyhdl eventually, but then decided to play it=20 safe and just change the one I am sure requires it.. >=20 >> } >> =20 >> my $hdl =3D delete $reqstate->{hdl}; >> @@ -627,9 +628,10 @@ sub websocket_proxy { >> } elsif ($opcode =3D=3D 8) { >> my $statuscode =3D unpack ("n", $payload); >> $self->dprint("websocket received close. status code: '$statuscode'= "); >> - if ($reqstate->{proxyhdl}) { >> - $reqstate->{proxyhdl}->push_shutdown(); >> - } >> + if (my $proxyhdl =3D $reqstate->{proxyhdl}) { >> + $proxyhdl->{block_disconnect} =3D 1 if length $proxyhdl->{wbuf} = > 0; >> + $proxyhdl->push_shutdown(); >> + } >> $hdl->push_shutdown(); >> } elsif ($opcode =3D=3D 9) { >> # ping received, schedule pong >>=20 >=20