From mboxrd@z Thu Jan 1 00:00:00 1970
Return-Path: <f.weber@proxmox.com>
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 608308A91
for <pve-devel@lists.proxmox.com>; Mon, 6 Mar 2023 15:04:40 +0100 (CET)
Received: from firstgate.proxmox.com (localhost [127.0.0.1])
by firstgate.proxmox.com (Proxmox) with ESMTP id 38E8F21D2
for <pve-devel@lists.proxmox.com>; Mon, 6 Mar 2023 15:04:10 +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
for <pve-devel@lists.proxmox.com>; Mon, 6 Mar 2023 15:04:09 +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 EEAC546106
for <pve-devel@lists.proxmox.com>; Mon, 6 Mar 2023 15:04:08 +0100 (CET)
From: Friedrich Weber <f.weber@proxmox.com>
To: pve-devel@lists.proxmox.com
Date: Mon, 6 Mar 2023 15:03:14 +0100
Message-Id: <20230306140314.1150179-1-f.weber@proxmox.com>
X-Mailer: git-send-email 2.30.2
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
X-SPAM-LEVEL: Spam detection results: 0
AWL -0.543 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: [pve-devel] [PATCH widget-toolkit] fix #4421: ui: guard setProxy
against races of slow vs fast requests
X-BeenThere: pve-devel@lists.proxmox.com
X-Mailman-Version: 2.1.29
Precedence: list
List-Id: Proxmox VE development discussion <pve-devel.lists.proxmox.com>
List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=unsubscribe>
List-Archive: <http://lists.proxmox.com/pipermail/pve-devel/>
List-Post: <mailto:pve-devel@lists.proxmox.com>
List-Help: <mailto:pve-devel-request@lists.proxmox.com?subject=help>
List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pve-devel>,
<mailto:pve-devel-request@lists.proxmox.com?subject=subscribe>
X-List-Received-Date: Mon, 06 Mar 2023 14:04:40 -0000
Some UI components use `Ext.data.Store.setProxy` to change their
associated API endpoint URL in reaction to user input. One example is
`BackupView`, which calls `setProxy` when the user switches from
listing backups on storage A to listing backups on storage B. However,
if A is slow, the UI may receive the response for A *after* the
response for B. It will then display the contents of A as if they were
the contents of B, resulting in a UI inconsistency.
The reason is that `Ext.data.Store` still processes the slow response
for A, even though it is obsolete. This patch overrides the
responsible callback of `Ext.data.Store` to only process responses
belonging to the currently active proxy object. This should rule out
similar race conditions in all components that use the `setProxy` API.
In the above example, the patch results in the response for A being
ignored.
Ignored responses are logged to the browser console.
Note that this patch only concerns components that use `setProxy` for
changing API endpoints. Other components (e.g. those using
`proxy.setURL` for the same purpose) may be open to similar race
conditions.
Signed-off-by: Friedrich Weber <f.weber@proxmox.com>
---
The original report only concerns the backup view [1], where the race
condition is easy to trigger. While ruling out this particular race is
simple, I thought it would be worthwhile to rule out race condition of
this category for all components. Hence this patch. However, most of the
other races are much harder to trigger, so it may be questionable
whether a general fix is needed. So if wanted, I can alternatively
submit a patch that only fixes the backup view.
Also, there are several occurrences of the `proxy.setURL` or `proxy.url = ...`
patterns (see [1]) which are also susceptible to race conditions, and which
are not fixed by this patch. However, for those, I have not found a nice
solution that does not involve changing a lot of call sites. If wanted, I can
give it another try, or alternatively only submit patches for components for
which triggering the race conditions seems realistic.
[1] https://bugzilla.proxmox.com/show_bug.cgi?id=4421
src/Utils.js | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/src/Utils.js b/src/Utils.js
index f55b9a5..8a97487 100644
--- a/src/Utils.js
+++ b/src/Utils.js
@@ -1451,3 +1451,18 @@ Ext.define('Proxmox.Async', {
return new Promise((resolve, _reject) => setTimeout(resolve, millis));
},
});
+
+Ext.override(Ext.data.Store, {
+ // If the store's proxy is changed while it is waiting for an AJAX
+ // response, `onProxyLoad` will still be called for the outdated response.
+ // To avoid displaying inconsistent information, only process responses
+ // belonging to the current proxy.
+ onProxyLoad: function(operation) {
+ let me = this;
+ if (operation.getProxy() === me.getProxy()) {
+ me.callParent(arguments);
+ } else {
+ console.log(`ignored outdated response: ${operation.getRequest().getUrl()}`);
+ }
+ },
+});
--
2.30.2