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

Revert "Send unhandled exceptions to the user (#2484)" #2497

Merged
merged 2 commits into from Dec 18, 2021

Conversation

jneira
Copy link
Member

@jneira jneira commented Dec 17, 2021

Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

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

Can you reproduce the problem locally? I don't have a Windows machine.

Can you share a link to a CI job showing the problem? I looked at the original PR and I don't see any hanging.

Approving based on trust.

@jneira
Copy link
Member Author

jneira commented Dec 17, 2021

Can you reproduce the problem locally? I don't have a Windows machine.

I did not try yet, will do

Can you share a link to a CI job showing the problem? I looked at the original PR and I don't see any hanging.

Collecting here the links:

Approving based on trust.

thanks!

@jneira jneira added the merge me Label to trigger pull request merge label Dec 17, 2021
@pepeiborra
Copy link
Collaborator

I have looked at the 3 linked failures and I don't see any hanging. The jobs seem to be taking a very long time to build ghcide and then the ghcide test suite, which suggests a problem with CI caching?
I am out of touch with the CI setup after the recent changes, but I don't think we should be reverting this PR without more evidence.

@jneira jneira removed the merge me Label to trigger pull request merge label Dec 17, 2021
Copy link
Collaborator

@pepeiborra pepeiborra left a comment

Choose a reason for hiding this comment

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

Withholding approval until we have more evidence that this PR caused hangs

@jneira
Copy link
Member Author

jneira commented Dec 17, 2021

I've added a fourth link to the original pr last failing run before the merge in master.

Not sure what additional evidence can I include apart the already linked:

  • the original pr failing run the ghcide test suite: cancelled after 4h 25 min in the middle of initialize response capabilities test group
  • a posterior build with the pr included cancelled after 2h 9m hanged in the same test group
  • one build in my fork with the commit included, cancelled after 4h 39m hanged in the same test group.
  • the successful run in my fork reversing the pr
  • the successful run of this pr reverting the pr

do you thin a local reproduction would be needed? even if i don't get to reproduce it locally it would not confirm than the pr is not the culprit and we need the ci green

@jneira
Copy link
Member Author

jneira commented Dec 17, 2021

to check if the cache is related (don't see how the cache can make hang a test at runtime but still) i am gonna open another pr invalidating it

@pepeiborra
Copy link
Collaborator

Is it the "text doc sync" test that is handling? Please try a local repro and share the log of messages

@jneira
Copy link
Member Author

jneira commented Dec 17, 2021

all runs, the failing and successful ones are using the same cache so...

@michaelpj
Copy link
Collaborator

Hmm, maybe this is actually a caching issue? It does seem like there's more recompilation going on that I would expect if the caching was working.

@jneira
Copy link
Member Author

jneira commented Dec 17, 2021

Is it the "text doc sync" test that is handling? Please try a local repro and share the log of messages

I am not at home right now but I ve opened a pr in my repo with LSP msgs enabled in the first call to tests

jneira#57

@jneira
Copy link
Member Author

jneira commented Dec 17, 2021

The run https://github.com/haskell/haskell-language-server/runs/4561717143?check_suite_focus=true
from the pr #2499 with a fresh cache has failed with the same hang so i think it is clear the cache is not related with the bug

@jneira
Copy link
Member Author

jneira commented Dec 17, 2021

I think is is clear this revert unblocks the ci and there are several prs in the queue to be merged.
We could continue working in the diagnose and the fix afterwards.

The pr with lsp messages enabled has ended, the full log can be downloaded here: https://github.com/jneira/haskell-language-server/suites/4693272570/logs?attempt=1

@pepeiborra pepeiborra merged commit 5d2189c into haskell:master Dec 18, 2021
pepeiborra added a commit that referenced this pull request Dec 18, 2021
pepeiborra added a commit that referenced this pull request Dec 19, 2021
pepeiborra added a commit that referenced this pull request Dec 19, 2021
* 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
@soulomoon
Copy link
Collaborator

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.

None yet

4 participants