Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Fix lint tool cancellation #1704

Merged
merged 4 commits into from
Jun 3, 2018

Conversation

doxxx
Copy link
Contributor

@doxxx doxxx commented Jun 1, 2018

When a lint tool takes a non-trivial amount of time to execute (e.g. gometalinter), and the user has lintOnSave enabled, saving a file twice in quick succession causes the previous lint tool execution to be cancelled.

However, subsequent executions of the lint tool will be terminated immediately because the "global" CancellationTokenSource used for cancelling the lint tool is still in the cancelled state.

This is fixed by recreating the CancellationTokenSource after it has been cancelled.

Copy link
Contributor

@ramya-rao-a ramya-rao-a left a comment

Choose a reason for hiding this comment

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

Nice catch! Can you do the same in the goVet.ts file for the vetting process?

@doxxx
Copy link
Contributor Author

doxxx commented Jun 2, 2018

It's not clear to me whether I need to wrap the cancel in a try/finally so that the dispose will always be called. I don't know how closely vscode's implementation of CancellationTokenSource mirrors that of .NET's.

@ramya-rao-a
Copy link
Contributor

I spent a long time looking at how the CancellationTokenSource works and came at the below conclusion:

  • Yes, like you figured out, once cancelled, the token remains in a cancelled state and every new onCancellationRequest handler will get called
  • The token source needs to be disposed after each use. Take the case of a completed lint process followed by a cancelled one. If the token source is not disposed after the first use, then the onCancellationRequest handler will get called even for the completed process

7f8e68d should take care of both cases.

It's not clear to me whether I need to wrap the cancel in a try/finally so that the dispose will always be called.

Are you talking about the case that one of the cancel event handlers might throw an exception and so the dispose won't be called?

@doxxx
Copy link
Contributor Author

doxxx commented Jun 3, 2018

Are you talking about the case that one of the cancel event handlers might throw an exception and so the dispose won't be called?

Yes.

@ramya-rao-a
Copy link
Contributor

Just tried throwing an exception from the cancel event handler. It doesn't affect the dispose. So we are good.

@ramya-rao-a ramya-rao-a merged commit 7922389 into microsoft:master Jun 3, 2018
@doxxx doxxx deleted the fix-lint-tool-cancellation branch October 31, 2018 14:18
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants