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: check for failed state during session log stream to prevent unbounded loop #564

Merged
merged 4 commits into from
Oct 24, 2023

Conversation

smorrisj
Copy link
Contributor

Summary
Check for failed state when streaming the session log to avoid attempting to check for logs in an unbounded loop.

Testing
Steps in #562

@smorrisj smorrisj changed the title fix: check for failed state to prevent infinite wait fix: check for failed state to prevent unbounded loop Oct 11, 2023
@smorrisj smorrisj changed the title fix: check for failed state to prevent unbounded loop fix: check for failed state during session log stream to prevent unbounded loop Oct 11, 2023
@@ -204,6 +204,7 @@ export class ComputeSession extends Compute {
"warning",
"completed",
"idle",
"failed",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

failed is one of the possible job states that indicates that the request is finished. Add it here so that the while loop will properly terminate on failed jobs.

@smorrisj smorrisj marked this pull request as ready for review October 11, 2023 16:56
@smorrisj
Copy link
Contributor Author

smorrisj commented Oct 11, 2023

I noticed that after fixing this, we get a rather ugly looking stringified json object rendered in the error message toast. This is due to how we're currently showing error messages for the run command.As part of this PR I'd propose that we come up with a way to clean this up a little. There are a few different error models that we could encounter at this level:

  • Axios request errors
  • Axios response errors
  • Axios error in setting up the request (not the same as the first issue above)
  • General typescript/node errors

Based on these possible states it seems like we might need some sort of well known translator that knows how to deal with the possibly different types coming in. Thoughts?

I've looked into making the error display in a modal error message, but it looks fairly different than the usual toast messages. The other tricky part specifically in the expired license case is that the context of the error that tells the user what's going on is actually in the details array, whereas the message value on that case is not very clear.

VSCode's error dialog in the modal option does allow for passing details in a details array. One such approach might be to detect if we are dealing with an error representation, if we are, then see if there are details. On the case of a details array we'd use the modal form. In the case of empty or undefined details array we'd use the normal brief toast message approach. Thoughts?

@smorrisj
Copy link
Contributor Author

smorrisj commented Oct 11, 2023

Before State:
image

Example of what the modal dialog for expired license would look like. Message is the top string and the details array are joined in the detail text below:

image

Accompanying (rough) code concept out at: main...vscode-modal-error

Copy link
Member

@scnwwu scnwwu 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 to me

@scnwwu
Copy link
Member

scnwwu commented Oct 12, 2023

VSCode's error dialog in the modal option does allow for passing details in a details array. One such approach might be to detect if we are dealing with an error representation, if we are, then see if there are details. On the case of a details array we'd use the modal form. In the case of empty or undefined details array we'd use the normal brief toast message approach. Thoughts?

Modal dialog looks good to me. I guess the most tricky part is where's the real detail. I remembered some errors where details is also complex objects.

@smorrisj
Copy link
Contributor Author

@scnwwu could I trouble you for another review? I pushed some code that switches over to the modal dialog approach discussed further up.

@Zhirong2022 Zhirong2022 added testing Test the pull requests and removed verification-needed labels Oct 19, 2023
@Zhirong2022
Copy link
Collaborator

"Connecting to SAS Session" will not hang when attempting to connect to an expired deployment. And now it will show error 'Request failed with status code 400'.

@Zhirong2022 Zhirong2022 added testing-complete Complete the pull requests testing and removed testing Test the pull requests labels Oct 24, 2023
@smorrisj smorrisj merged commit b70da81 into main Oct 24, 2023
1 check passed
@Zhirong2022 Zhirong2022 added ready for release Code pushed, but not released in VS code marketplace yet and removed testing-complete Complete the pull requests testing labels Oct 26, 2023
@tianliang657 tianliang657 added the interactive Issue could not be covered by automation test label Dec 8, 2023
@scnwwu scnwwu deleted the bug/562 branch February 21, 2024 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
interactive Issue could not be covered by automation test ready for release Code pushed, but not released in VS code marketplace yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

"Connecting to SAS Session" hang when attempting to connect to an expired deployment
4 participants