Skip to content

Demote no access to the persisted file to debug messages #51

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

Merged

Conversation

fendor
Copy link
Collaborator

@fendor fendor commented Nov 15, 2019

It is not really an error as it may happen if the file you want to lint had been closed before the lint can be applied.

@fendor fendor mentioned this pull request Nov 15, 2019
26 tasks
@fendor fendor requested a review from lukel97 November 15, 2019 11:50
@fendor
Copy link
Collaborator Author

fendor commented Nov 15, 2019

@jneira with these changes, you should no longer see messages such as lintCmd: no access to the persisted file.!

@mpickering
Copy link
Owner

It could be argued that it's still an error if file was requested before it was inserted into the VFS and it's certainly an error to request a file which was never inserted into the VFS?

@fendor
Copy link
Collaborator Author

fendor commented Nov 15, 2019

That is true. Unforutantely, we can not tell apart the cases right? Closing a file, removes it from the VFS, the lintCmd is stalled by runActionWithContext. So, opening a file then realizing you dont need it, closing it again, will show the "error" message to the user, although it is perfectly fine.

@lukel97
Copy link
Collaborator

lukel97 commented Nov 15, 2019

Should these pending requests not end up being cancelled whenever the file is closed?

@fendor
Copy link
Collaborator Author

fendor commented Nov 15, 2019

@bubba They are not, because the following might happen:
Open file in Editor, it gets added to the VFS -> request lint diagnostics.
Submit this to the scheduler. Scheduler schedules it and executes runActionWithContext which builds the context of the FilePath that we want diagnostics for. This might take a while since also dependency builds may be triggered.
Now the file in the editor is closed and the file is removed from the VFS.
However, we have no way of aborting building the dependencies, e.g. runActionWithContext is never aborted. After runActionWithContext initialises the context, the plugin hlint is executed to get the diagnostics which tries to access the temp file, but fails since the virtual file has already been removed.

@lukel97
Copy link
Collaborator

lukel97 commented Nov 16, 2019

@fendor makes sense, these are all in IdeGhcM. But I thought we already had some logic to cancel requests that were in flight, at least whenever LSP sends a cancelRequest notification. Could this be called for any requests operating on a certain file that the VFS is told to close?

cancelRequest :: forall m . Scheduler m -> J.LspId -> IO ()
cancelRequest Scheduler { requestsToCancel, requestsInProgress } lid =
STM.atomically $ do
wip <- STM.readTVar requestsInProgress
when (Set.member lid wip)
$ STM.modifyTVar' requestsToCancel (Set.insert lid)

This could be made an issue or sorted out after this though

@fendor fendor force-pushed the demote-with-mapped-file-error-msg branch from d2db1cd to a5c749b Compare November 17, 2019 12:17
@fendor
Copy link
Collaborator Author

fendor commented Nov 17, 2019

@bubba Does cancelRequest cancel actions that are already happening? Because that is the "problem" the action is already being executed, just stuck in runActionWithContext.

@lukel97
Copy link
Collaborator

lukel97 commented Nov 17, 2019

@fendor Just actions that are queued from what I can tell

@jneira
Copy link

jneira commented Nov 17, 2019

Well, as far i can test the alerts are gone from vscode and i can see them in hie.log. Thanks @fendor!

@fendor fendor merged commit 532aa60 into mpickering:hie-bios Nov 18, 2019
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.

5 participants