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.

Sign up for GitHub

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

Jump to bottom

Send unhandled exceptions to the user #2484

Merged
merged 5 commits into from
Dec 16, 2021
Merged

Send unhandled exceptions to the user #2484

merged 5 commits into from
Dec 16, 2021

Conversation

pepeiborra
Copy link
Collaborator

@pepeiborra pepeiborra commented Dec 13, 2021 •
edited by gitpod-io bot
Loading

Closes #2390

michaelpj
michaelpj approved these changes Dec 13, 2021
View reviewed changes
ghcide/src/Development/IDE/LSP/LanguageServer.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/LSP/LanguageServer.hs Outdated Show resolved Hide resolved
ghcide/src/Development/IDE/LSP/LanguageServer.hs Show resolved Hide resolved
@pepeiborra pepeiborra added the merge me Label to trigger pull request merge label Dec 13, 2021
@jneira
Copy link
Member

jneira commented Dec 15, 2021 •
edited
Loading

The test suite for tactics has failed twice several times for windows 8.10.7, with different test error cases, not sur if it will be transient

This cancelled job had 7 attempts and the tactics test suite failed in the 6 first ones

@Anton-Latukha
Copy link
Collaborator

Anton-Latukha commented Dec 15, 2021 •
edited
Loading

Recent failed test workflows were:
pepeiborra - https://github.com/haskell/haskell-language-server/runs/4513326877?check_suite_focus=true
AntonLatukha - https://github.com/haskell/haskell-language-server/runs/4527885363?check_suite_focus=true (before that I & CI skipped my previous restart of a workflow, because right after it I decided not just restart, but also do a sync at the same time)
jneira - https://github.com/haskell/haskell-language-server/runs/4530467808?check_suite_focus=true

@pepeiborra
Copy link
Collaborator Author

The last failure is due to the old sqlite3 hiedb problem, not something specific to this change. We have a ticket for it somewhere, but I am on my phone right now and can't look.

We have a couple of sources of flakiness, and I would appreciate if someone would find the time to look into the hiedb one. Why is it getting triggered? Is the test suite running tests in parallel?

SQLite3 returned ErrorBusy while attempting to perform step: database is locked\n",

@pepeiborra
Copy link
Collaborator Author

Added code to stop the hiedb on shutdown. Might or might not help

@pepeiborra pepeiborra requested a review from michaelpj December 16, 2021 10:50
michaelpj
michaelpj approved these changes Dec 16, 2021
View reviewed changes
ghcide/src/Development/IDE/LSP/LanguageServer.hs Show resolved Hide resolved
ghcide/src/Development/IDE/LSP/LanguageServer.hs Show resolved Hide resolved
ghcide/src/Development/IDE/LSP/LanguageServer.hs
@@ -190,15 +194,22 @@ runLanguageServer options inH outH getHieDbLoc defaultConfig onConfigurationChan
pure $ Right (env,ide)


-- | Runs the action until it ends or until the given MVar is put.
-- Rethrows any exceptions.
untilMVar :: MonadUnliftIO m => MVar () -> m () -> m ()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Kind of surprised these sorts of utilities aren't available elsewhere. I couldn't find them, anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Especially the pattern of forking a thread with an MVar to control terminating it 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

They may exist, I'm not sure either.

@michaelpj
Copy link
Collaborator

The windows jobs seemed to be hanging (stuck for 4h). I attempted to cancel them but it's unclear that it's worked.

@mergify mergify bot merged commit 0211f75 into master Dec 16, 2021
@michaelpj
Copy link
Collaborator

... how did that work?

@jneira
Copy link
Member

jneira commented Dec 16, 2021

... how did that work?

it should not 😕
the test post-jobs was supposed to be cancelled but it was not
https://github.com/haskell/haskell-language-server/runs/4549724494?check_suite_focus=true

@michaelpj
Copy link
Collaborator

I think this might be making the ghcide tests hang on windows. I think we should revert it until we can confirm.

@jneira
Copy link
Member

jneira commented Dec 17, 2021 •
edited
Loading

I think this might be making the ghcide tests hang on windows. I think we should revert it until we can confirm.

well i have a pr fixing the cancelling thing and including this pr, if its tests are not stuck it would be fine

After reading your comment in #2493 it seems so
I am gonna revert it in my fork to check it, i have a pr in my fork with is also stuck: jneira#55 so the problem is reproduced there too

jneira added a commit to jneira/haskell-language-server that referenced this pull request Dec 17, 2021
@jneira
Revert "Send unhandled exceptions to the user (haskell#2484)"
edd544e 
This reverts commit 0211f75.
@jneira
Copy link
Member

jneira commented Dec 17, 2021

pr in my fork with this reverted jneira#56

@jneira
Copy link
Member

jneira commented Dec 17, 2021

Ok the pr in my fork with this reverted has passed the ghcide tests: https://github.com/jneira/haskell-language-server/runs/4559275575?check_suite_focus=true
I am gonna open here

@jneira jneira mentioned this pull request Dec 17, 2021
Merged
pepeiborra pushed a commit that referenced this pull request Dec 18, 2021
@jneira
Revert "Send unhandled exceptions to the user (#2484)" (#2497)
5d2189c 
This reverts commit 0211f75.
pepeiborra added a commit that referenced this pull request Dec 18, 2021
@pepeiborra
Revert "Revert "Send unhandled exceptions to the user (#2484)" (#2497)"
6968b80 
This reverts commit 5d2189c.
pepeiborra added a commit that referenced this pull request Dec 19, 2021
@pepeiborra
Revert "Revert "Send unhandled exceptions to the user (#2484)" (#2497)"
3c65b5c 
This reverts commit 5d2189c.
pepeiborra added a commit that referenced this pull request Dec 19, 2021
@pepeiborra
Unhandled exceptions fixed (#2504)
c0d8a3b 
* Revert "Revert "Send unhandled exceptions to the user (#2484)" (#2497)"

This reverts commit 5d2189c.

* Log when reactor thread exits

* log shakeSessionInit

* Do not assume that the build has been initialized
@eddiemundo eddiemundo mentioned this pull request Dec 23, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelpj michaelpj michaelpj approved these changes

Assignees
No one assigned
Labels
merge me Label to trigger pull request merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Report error conditions via LSP message dialogs
4 participants
@pepeiborra @jneira @Anton-Latukha @michaelpj