Skip to content

Progress reporting improvements#1784

Merged
mergify[bot] merged 18 commits into
masterfrom
progressreport-redo
May 3, 2021
Merged

Progress reporting improvements#1784
mergify[bot] merged 18 commits into
masterfrom
progressreport-redo

Conversation

@pepeiborra

@pepeiborra pepeiborra commented May 2, 2021

Copy link
Copy Markdown
Collaborator

Rework of #1770. Summary of changes:

  1. progress reporting logic extracted into its own module
  2. Removed one layer of async handling, reducing the chances of race conditions
  3. Correctly implement the LSP spec for window/workDoneProgress/create by waiting until the client has responded
  4. Fix bugs: division by zero (when todo is 0) and handling when NoProgress
  5. Performance: the reporting loop is now O(1) instead of O(N) in the number of files

I think this is the best we can do to close #1749 while sharing the same logic for both tests and app code

@pepeiborra
pepeiborra requested a review from wz1000 May 2, 2021 10:50
@pepeiborra
pepeiborra force-pushed the progressreport-redo branch from b3ba386 to 793be94 Compare May 2, 2021 10:56
@pepeiborra
pepeiborra requested a review from ndmitchell May 2, 2021 10:57
@pepeiborra
pepeiborra marked this pull request as ready for review May 2, 2021 14:36

@ndmitchell ndmitchell left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I see a few updates to the test - are these fixing race conditions that this patch showed up?

This code is way clearer!

Comment thread hls-test-utils/src/Test/Hls.hs Outdated
Comment thread hls-test-utils/src/Test/Hls.hs
Comment thread ghcide/src/Development/IDE/Core/ProgressReporting.hs Outdated
Comment thread ghcide/src/Development/IDE/Core/ProgressReporting.hs
Comment thread ghcide/src/Development/IDE/Core/ProgressReporting.hs Outdated
Comment thread ghcide/src/Development/IDE/Core/ProgressReporting.hs
Comment thread ghcide/src/Development/IDE/Core/Shake.hs
Comment thread ghcide/src/Development/IDE/Core/ProgressReporting.hs Outdated
@pepeiborra

Copy link
Copy Markdown
Collaborator Author

I see a few updates to the test - are these fixing race conditions that this patch showed up?

I fixed a bunch of tests in the previous version of this PR and decided to preserve the fixes. Not necessarily race conditions: often tests rely on sketchy invariants, e.g. a test that waits for three logging notifications before checking if the build session is restarted is going to break for reasons unrelated to the property it's checking.

@pepeiborra
pepeiborra force-pushed the progressreport-redo branch from 7c40949 to 7fc92b1 Compare May 3, 2021 08:01
@pepeiborra
pepeiborra force-pushed the progressreport-redo branch from 7fc92b1 to 3850495 Compare May 3, 2021 08:13
@pepeiborra pepeiborra added the merge me Label to trigger pull request merge label May 3, 2021
@mergify
mergify Bot merged commit 6ecf17b into master May 3, 2021
@pepeiborra pepeiborra mentioned this pull request May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge me Label to trigger pull request merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Progress messages logic might have race conditions

2 participants