NatLibFi / Skosmos

Thesaurus and controlled vocabulary browser using SKOS and SPARQL
Other
229 stars 94 forks source link

Race conditions in Skosmos 3 AJAX requests #1561

Closed osma closed 8 months ago

osma commented 11 months ago

URL address of the page where you encountered the problem

http://localhost/Skosmos/yso/en/

Description of the problem

When quickly clicking on entries in the alphabetical index, it's possible to cause a situation where more than one request is happening in parallel. This may cause a situation where the wrong result is shown to the user.

This happened recently with a Cypress test for the alphabetical index; see https://github.com/NatLibFi/Skosmos/pull/1558#issuecomment-1822927410

image

Details from above:

It looks like this test revealed a race condition in the alphabetical index Vue component. It starts loading the index entries for >the letter A, but the user (here simulated by Cypress) clicks on D before that operation has finished. This leads to a race >condition with both requests (for A and D) happening in parallel, and depending on the order in which they finish, the index will >contain either 8 or 33 entries. The test expects 8, so it will fail if there are 33 entries.

This is probably a more general problem with all kinds of asynchronous operations performed by Vue components. We should >have some sort of queue mechanism, or a way to cancel/invalidate previous operations. In this case, when the user clicks on D >before the request for A has finished, it would make sense to cancel the request for A.

I think we need some kind of mechanism to ensure that there are not more than one similar AJAX-style requests happening simultaneously. In Skosmos 2, a queue for AJAX requests was used to solve similar problems; see https://github.com/NatLibFi/Skosmos/pull/1558#issuecomment-1822941901

Although this happened only in the alphabetical index component, the problem is more general and also applies to partial page loads triggered from e.g. the alphabetical index and the hierarchy component. Also in that case, it is possible to click on many entries so quickly that multiple requests are "in the air" at the same time and they could finish in any order. So we need a general mechanism to ensure that only the last action (last click) determines the outcome; probably the old request should be cancelled when an user action triggers a new request of the same kind.

osma commented 8 months ago

I think this could be done using an AbortController to abort the previous fetch operation.

Here's what Phind.com suggested:

To modify the partialPageLoad function to cancel the previous fetch that hasn't yet finished using an AbortController, follow these steps:

  1. Create an AbortController instance outside the function scope to ensure it persists across multiple function calls. This allows you to abort the previous fetch when a new one is initiated.
  2. Pass the signal from the AbortController instance to the fetch request as an option. This associates the fetch request with the controller, enabling you to abort it later.
  3. Call the abort method on the AbortController instance before initiating a new fetch. This cancels the previous fetch if it's still in progress.
  4. Handle the potential AbortError in the fetch promise chain to prevent unhandled promise rejection errors when a fetch is aborted.

The challenge here (at least from my perspective as a Vue noob) is where to keep the AbortController instance(s) between calls to partialPageLoad. Somewhere in the Vue application or component? In a global variable? Somehow inside the partialPageLoad function?

Here is one recipe based on the (now abandoned) vue-resource library.