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]:299 - uncaught exception session with id 'Not Found' after dele… #313

Merged
merged 6 commits into from
Sep 30, 2024

Conversation

BeRecursive22
Copy link
Contributor

@BeRecursive22 BeRecursive22 commented Sep 19, 2024

…ting an srcbook

fix: Handle session not found errors in TsServer event handlers

  • Added try-catch blocks in onSemanticDiag and onSuggestionDiag handlers
  • Gracefully handle and log errors when findSession fails
  • Prevent server crashes due to unhandled session not found errors

'@srcbook/api': patch
---

fix(api): Handle session cleanup after srcbook deletion. Resolved an issue where deleting an srcbook would lead to an uncaught exception due to an orphaned session. The fix ensures proper cleanup of associated sessions when an srcbook is deleted, preventing the "Session with id not found" error.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we simplify this message to something like "Fix uncaught exceptions that can occur after Srcbook deletion" ?

Copy link
Contributor

@benjreinhart benjreinhart Sep 19, 2024

Choose a reason for hiding this comment

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

This description is better placed in the PR description. Then here we can have a short message which will end up in the Changelog (which will link back to the PR)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@benjreinhart sure. But it seems like the issue is much bigger than what I anticipated previously. It looks like a race condition going on between tsserver and actual session deletion which is leading to this error

Copy link
Contributor Author

@BeRecursive22 BeRecursive22 Sep 19, 2024

Choose a reason for hiding this comment

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

@benjreinhart this simple order change (which I did above) of session deletion won't make it go away.

The issue, I believe is, not properly handling the errors for onSemanticDiag and onSuggestionDiag event when the session id is not found.

image

When I added extra try...catch blocks, the server didn't crash. I tried deleting multiple srcbooks and the server didn't crash.

I'll make the changes and push it.

@@ -124,6 +124,7 @@ async function cellExec(payload: CellExecPayloadType) {
const session = await findSession(payload.sessionId);
const cell = findCell(session, payload.cellId);

console.log('inside cellExec');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove this?

@Lokendra-sinh
Copy link

@nichochar can we approve the workflow or is there anything else I need to take care of?

@nichochar
Copy link
Contributor

Approved, this is looking good to go!

@benjreinhart benjreinhart merged commit a063c46 into srcbookdev:main Sep 30, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants