Re: [pve-devel] [PATCH widget-toolkit] fix #4421: ui: guard setProxy against races of slow vs fast requests

[Dominik Csapak <d.csapak@proxmox.com>]

On 3/7/23 19:49, Thomas Lamprecht wrote:
> Am 06/03/2023 um 15:03 schrieb Friedrich Weber:
>> 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.
> 
> IMO a general fix/future proofing can be OK, so besides a small nit inline:
> LGTM, but did not checked/tested this too closely - @Dominik, what do you
> think on this?
> 

This change is non-intrusive enough that it's OK, since it
fixes the reported issue and potentially some more.

When we're only fixing the one reported place, i guess
sooner or later someone else reports another instance of this,
and by then we probably forgot that we fixed it already once ;)

Really fixing all points where something like that can happen is
not easy since most of them are using Proxmox.Utils.API2Request
instead of a store, or as Friedrich already wrote, setting
the URL of the proxy manually, so this seems to be good
middle ground for now.