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): don't reload the page until _after_ deletion #11711

Merged
merged 1 commit into from
Sep 4, 2023

Conversation

agilgur5
Copy link
Contributor

@agilgur5 agilgur5 commented Aug 29, 2023

Hopefully fixes #11661

Motivation

Modifications

  • modify the code to actually wait on the requests

    • use Promise.all to run the delete and archive delete simultaneously
  • partially rewrite in newer async/await syntax as it is considerably cleaner that way

    • otherwise, the scoping/conditionals get pretty confusing and require some redundant code
    • ideally, we should more generally rewrite Promise-based code to async/await style syntax, which is generally cleaner & preferred by the community over Promises or callback hell
  • refactor the conditionals to have less nesting and early return instead

Verification

Build passes. UI still works.
I was unable to reproduce the original problem, so as a result I can't verify that this fixes the issue completely, but the evidence made my hypothesis seem likely.

Future Work

Per above, refactor to have less nesting and use async/await

- the deletions happen asynchronously and were not waited on
  - this meant that the error check could occur too early
  - and that the page reload might occur too early, before the requests finished
    - in particular, this scenario could happen on a higher latency connection (e.g. on a remote server, not running locally)
      - I and others were not able to reproduce locally, but Pipekit reliably reproduced it
    - if this were to happen, the UI would be dropping its connection and reconnecting during the reload
      - if the connection were dropped while the request were in-flight, the server could drop the request entirely
        - based on Server logs from Pipekit, a 408 status code suggested that this might in fact be the case
        - if the request were dropped before the response, it is possible that the server may not have completed deletion
          - e.g. may have not yet sent a request to the k8s API server, or drops the request to the k8s API server, etc

- modify the code to actually wait on the requests
  - use `Promise.all` to run the delete and archive delete simultaneously
- partially rewrite in newer async/await syntax as it is considerably cleaner that way
  - otherwise, the scoping/conditionals get pretty confusing and require some redundant code
  - ideally, we should more generally rewrite Promise-based code to async/await style syntax, which is generally cleaner & preferred by the community over Promises or callback hell

- refactor the conditionals to have less nesting and early return instead
  - per review comments I made on f18b339's PR
  - there's several places where we can rewrite to have less nesting. async/await will also help with that on the UI

Signed-off-by: Anton Gilgur <agilgur5@gmail.com>
Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tico24 Could you test the changes to see if that fixes your issue?

@terrytangyuan
Copy link
Member

cc @rbreeze to help review

Copy link
Member

@rbreeze rbreeze left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending testing

@agilgur5
Copy link
Contributor Author

agilgur5 commented Aug 30, 2023

This is a fix (and generally good change) even if it does not fully resolve #11661, so I would say it is safe to merge in regardless.

@tico24
Copy link
Member

tico24 commented Aug 30, 2023

This is on my todo list to test, I just haven't had the time today.

@tico24
Copy link
Member

tico24 commented Sep 4, 2023

This does fix issue #11661, thank you.

@terrytangyuan terrytangyuan merged commit 9e7dc25 into argoproj:master Sep 4, 2023
22 checks passed
@agilgur5 agilgur5 deleted the fix-ui-delete-workflow branch September 4, 2023 16:13
@agilgur5
Copy link
Contributor Author

agilgur5 commented Sep 6, 2023

Future Work

Per above, refactor to have less nesting and use async/await

Created #11740 to track refactor to async/await

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Deleting a workflow on the workflow details page doesn't do anything
4 participants