Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ui): ListWatch should not _both_ set and depend on nextOffset #12672

Merged

Conversation

agilgur5
Copy link
Member

@agilgur5 agilgur5 commented Feb 16, 2024

Fixes #12663, fixes #12626 (as well as #12025 (comment) 2.i, the one with the browser network tab screenshot)
Follow-up to #11891

Motivation

The ListWatch previously both depended on and set nextOffset, which was recursive

  • note that it technically does depend on it since the list uses the whole pagination object, but nextOffset only changes within the effect or at the same time as another pagination
    • so it is safe to remove as a dep
  • when pagination was enabled, this recursive dep/set would cause a duplicate request
    • in my repro, I only had one duplicate request, as after that nextOffset stayed consistent. would have two for each page move
    • the quick succession of these duplicates also somehow made some of the SSEs impossible(???) to stop; even calling listWatch.stop() would not stop them
      • this would result in SSEs continuously growing until the browser connection limit was hit and then no new connections being allowed, halting all network activity
      • notably, I confirmed that the effect clean up and listWatch.stop() was occurring (through logs and other debugging), it just seemed to not close the SSE for some reason (???)
        • I have not been able to root cause that, but this fix makes that problem disappear for now at least
          • it might be an RxJS issue (notably there is some usage of deprecated functions) or a Server issue, or an SSE issue, not sure

Modifications

  • remove nextOffset from the ListWatch's useEffect's deps array

  • also fix a typo I had in the query param retrieval of offset

    • I had accidentally written the literal name instead of putting the actual name of the param, offset, woops 😕
  • plus consistently ensure that the pagination defaults are undefined everywhere, so that the useEffect deps do not change

    • removes more unnecessary network requests / compute / memory / etc
      • esp. with the quick succession of SSEs problem above, definitely want to limit this to only as much as is needed

Verification

In my partial repro, the network waterfall looked like the below, with duplicate SSEs that were not always cancelled:

Screenshot 2024-02-15 at 1 54 28 PM

After this fix, it now looked as intended, only one SSE at a time:

Screenshot 2024-02-16 at 12 38 45 AM

- that was recursive, so when pagination was enabled, it would cause a duplicate request
  - note that it technically _does_ depend on it since the `list` uses the whole `pagination` object, but `nextOffset` only changes within the effect or at the same time as another pagination
    - so it is safe to remove as a dep
  - in my repro, I only had one duplicate request, as after that `nextOffset` stayed consistent. would have two for each page move
    - for other folks, this seemed to have caused an infinite loop, but I was not able to repro that specifically
  - this quick succession of these duplicates also somehow made some of the SSEs impossible to stop; even calling `listWatch.stop()` would not stop them
    - this would result in SSEs continuously growing until the browser connection limit was hit and then no new connections being allowed, halting all network activity
    - notably, I confirmed that the effect clean up and `listWatch.stop()` was occurring, it just seemed to not close the SSE for some reason
      - I have not been able to root cause that, but this fix makes that problem disappear for now at least

- also fix a typo I had in the query param retrieval of `offset`
  - I had accidentally written the literal `name` instead of putting the actual name of the param, `offset`, woops

- plus consistently ensure that the pagination defaults are `undefined` everywhere, so that the `useEffect` deps do not change
  - removes more unnecessary network requests / compute / memory / etc
    - esp. with the quick succession of SSEs problem above, definitely want to limit this to only as much as is needed

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
@isubasinghe isubasinghe merged commit 030d581 into argoproj:main Feb 17, 2024
20 checks passed
@agilgur5 agilgur5 deleted the fix-ui-wf-pagination-dupe-requests branch February 17, 2024 08:03
isubasinghe pushed a commit to isubasinghe/argo-workflows that referenced this pull request Feb 27, 2024
isubasinghe pushed a commit to isubasinghe/argo-workflows that referenced this pull request Feb 28, 2024
…argoproj#12672)

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Signed-off-by: Isitha Subasinghe <isubasinghe@student.unimelb.edu.au>
@agilgur5 agilgur5 added this to the v3.5.x patches milestone Apr 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ui prioritized-review For members of the Sustainability Effort
Projects
None yet
Development

Successfully merging this pull request may close these issues.

UI: Workflow list blocked when limit applied UI infinite SSE creation + timeout on Chrome
2 participants