Fix race condition in worksheet output #5552
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
smarter
approved these changes
Dec 2, 2018
vscode-dotty/src/worksheet.ts
Outdated
| actualLine += 1 | ||
| } | ||
| }) | ||
| const _ = await vscode.workspace.applyEdit(addNewLinesEdit); |
There was a problem hiding this comment.
this will actually create a value named _, I think you can just do await vscode.workspace.applyEdit(addNewLinesEdit)
Duhemm
force-pushed
the
fix/worksheet-race-condition
branch
from
cdb4c76
to
ae2f513
Compare
Sometimes, the `ApplyEdits` that we use in the worksheet to insert blank lines so that the worksheet output fits were reordered. The lead to some output of the worksheet being lost, with decorations being inserted after the end of the file. We know keep track of the edits that we want to apply to the worksheet and ensure that they are executed in the same order as the messages were received.
In 23785e6, we moved worksheet.run() from didSave to willSave since didSave is not called when the document is not dirty, unfortunately this gives the wrong behavior when the document _is_ dirty: it means we're going to send a run worksheet request to the server before the server has seen all the changes to the document, in particular with multi-line output this means that the server will see the output with the added empty lines, which means the line numbers in worksheet/publishOutput will be wrong. To avoid this issue we now run the worksheet either on willSave or didSave depending on whether the document is dirty or not. To reproduce the issue, you can create a worksheet with: ``` val x = """ """ "hi" ``` in it, then save the document multiple times.
smarter
force-pushed
the
fix/worksheet-race-condition
branch
from
ae2f513
to
843d595
Compare
Duhemm
commented
Dec 3, 2018
There was a problem hiding this comment.
we're going to send a run worksheet
request to the server before the server has seen all the changes to the
document, in particular with multi-line output this means that the
server will see the output with the added empty lines, which means the
line numbers in worksheet/publishOutput will be wrong.
👍 👍 👍
smarter
added a commit
to dotty-staging/dotty
that referenced
this pull request
Dec 9, 2018
Even after scala#5552 there were still situations were the worksheet output was not displayed properly due to all the complications needed to handle multi-line output. Getting this right seems hopeless until vscode gets some better API (microsoft/vscode#63600), so instead in this commit we avoid the problem by only displaying the first line of multi-line output. The complete output is still available by hovering over the decoration which should be good enough.
smarter
added a commit
to dotty-staging/dotty
that referenced
this pull request
Dec 10, 2018
Even after scala#5552 there were still situations were the worksheet output was not displayed properly due to all the complications needed to handle multi-line output. Getting this right seems hopeless until vscode gets some better API (microsoft/vscode#63600), so instead in this commit we avoid the problem by only displaying the first line of multi-line output. The complete output is still available by hovering over the decoration which should be good enough.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Sometimes, the
ApplyEditsthat we use in the worksheet to insert blanklines so that the worksheet output fits were reordered. The lead to some
output of the worksheet being lost, with decorations being inserted
after the end of the file.
We know keep track of the edits that we want to apply to the worksheet
and ensure that they are executed in the same order as the messages were
received.