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 admin commands getting stuck #717

Merged
merged 3 commits into from
Nov 16, 2022
Merged

Conversation

AndrewFerr
Copy link
Member

The two main reasons why admin commands got stuck are:

  1. Calling yargs.parse multiple times apparently doesn't behave well when command handlers return a Promise:

Note: parse() should be called only once when command() is called with a handler returning a promise. If your use case requires parse() to be called several times, any asynchronous operation performed in a command handler should not result in the handler returning a promise.

  1. When yargs.parse throws, or the callback given to it throws, yargs.argv is permanently set to a YError, and all future calls to yargs.parse throw that same error.

This PR cleans up some of our yargs usage & moves all asynchronicity of command callbacks outside of yargs.parse.

@AndrewFerr AndrewFerr added javascript Pull requests that update Javascript code S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems. labels Nov 15, 2022
@AndrewFerr AndrewFerr requested a review from a team as a code owner November 15, 2022 07:52
@AndrewFerr AndrewFerr self-assigned this Nov 15, 2022
Copy link
Contributor

@jaller94 jaller94 left a comment

Choose a reason for hiding this comment

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

Looks good.

@AndrewFerr
Copy link
Member Author

The latest commit is needed to fix asynchronous commands not being awaited properly. It also moves error handling back into Main.ts to catch errors more generically & to minimize the size of the diff.

@AndrewFerr AndrewFerr merged commit 7e1aa50 into develop Nov 16, 2022
@AndrewFerr AndrewFerr deleted the af/fix-stuck-admin-commands branch November 28, 2022 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
javascript Pull requests that update Javascript code S-Major Severely degrades major functionality or product features, with no satisfactory workaround T-Defect Bugs, crashes, hangs, vulnerabilities, or other reported problems.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants