improvement: Only reindex workspace on onBuildTargetChanged #5737
Conversation
tgodzik
force-pushed
the
reconnect
branch
2 times, most recently
from
eefae0a
to
dd8709c
Compare
tgodzik
changed the title
|
|
||
| import bill.Bill | ||
|
|
||
| class BspBuildChangedSuite extends BaseLspSuite("bsp-build-changed") { |
There was a problem hiding this comment.
So this suite was testing exactly what I removed. I think the Scala CLI test makes much more sense.
Previously, we would always reconnect to build server, which is not needed and takes longer. Instead now we only reload and reindex the workspace.
| buildTool, | ||
| digest, | ||
| importBuild, | ||
| case Some(BuildTool.Found(buildTool: ScalaCliBuildTool, _)) |
There was a problem hiding this comment.
This is just a refactor to use BuildTool.Found with checksum ready.
| _ <- indexer.profiledIndexWorkspace(runDoctorCheck) | ||
| } { | ||
| focusedDocument().foreach(path => compilations.compileFile(path)) | ||
| compilers.cancel() |
There was a problem hiding this comment.
importBuild does that too, why do we need this again?
There was a problem hiding this comment.
Hmm... it wasn't working without it, I think we might actually need to move it as last.
There was a problem hiding this comment.
Right, looks like we should have cancelled after build import. Moved it there.
| params: b.DidChangeBuildTarget | ||
| ): Unit = { | ||
| // Make sure that no compilation is running, if it is it might not get completed properly |
There was a problem hiding this comment.
I am not sure, I could not reproduce it reliably. My guess is that when you send one compile first and then change using directives in another compile requests a short while later, last one will get cancelled and the first one forgotten.
There was a problem hiding this comment.
And get stuck in the queue until next cancel.
There was a problem hiding this comment.
So it's a BatchedFunction thing not a scala-cli thing?
There was a problem hiding this comment.
yes, but it showed up here multiple times when testing.
There was a problem hiding this comment.
By stuck in the queue to you mean that scala-cli won't send any response to that compile and the BatchedFunction queue is forever locked? If that's the case this is solid.
There was a problem hiding this comment.
Yeah, it seems Scala CLI doesn't always send the respond, looks like it's a race condition, but might be hard to fix.
|
Let's wait to merge it until after release to be on the safe side. |
Previously, we would always reconnect to build server, which is not needed and takes longer. Instead now we only reimport and reindex the workspace.
Related to VirtusLab/scala-cli#2412