Cancel future when process is canceled

KotlinIsland commented 2 years ago

First time contributor checklist

[x] I have read how to contribute to this project
[x] I have read the code of conduct to this project

Contributor checklist

[x] I am targeting the master branch (and not the release branch)
[x] I am using the provided codeStyleConfig.xml
[x] I have tested my contribution on these versions of PyCharm/IDEA:
- PyCharm Community 2021.3.1
- IntelliJ IDEA 2021.3.1
[x] My contribution is fully baked and ready to be merged as is
[x] I ensure that all the open issues my contribution fixes are mentioned in the commit message of my first commit using the Fixes #1234 syntax

Description

Ok, so basically I think the way that axync tasks are handled is kinda funky, I'm not 100% sure what's going on but I'm fairly certain that when the process manager cancels the process the checkCanceled method will throw a ProcessCanceledException and when it does we should then cancel the future, I believe this will improve performance.

Type of Changes

	Type
✓	:bug: Bug fix

leinardi commented 2 years ago

Hi @KotlinIsland, thank you for the PR. But just out of curiosity, did you measure any performance improvement or it is just a guess that this PR will improve performance?

KotlinIsland commented 2 years ago

I really can't remember tbh 😁.

About 6 months ago I messed all around with this plugin and made a bit of progress, but I can't remember what was what.

I think the issue this fixes is that the plugin uses 100% of the system resources or something. Because it's running hundreds of redundant instances of mypy on top of each other.

I'm probably not right though 😳.

Have you noticed this behaviour before?

leinardi commented 2 years ago

I see, but is this still relevant after #77? @intgr what do you think?

intgr commented 2 years ago

This code path isn't used in the current "real-time scan" logic ~~(I think it wasn't before #77 either).~~

But Async.whenFinished is still used in ScanFilesBeforeCheckinHandler, which is the "Before Commit" "Scan with Mypy" logic. If ProcessCanceledException can be thrown there, this change should indeed be an improvement.

intgr commented 2 years ago

It's well possible that this change is all that was needed to fix the performance issues, and the rewrite to the ExternalAnnotator API (which has some small downsides) wasn't necessary. :facepalm:

intgr commented 2 years ago

Anyway I think this is a worthwhile change and should be merged for the "Before Commit" code path at least.

After that it's possible to revert #77, although I'm not sure yet if we want to.

leinardi commented 2 years ago

After that it's possible to revert #77, although I'm not sure yet if we want to.

Yeah I'm not sure as well, but I have the feeling that the new implementation takes longer to show the violation compared to the old one. So, maybe, reverting would be worth. But needs to be tested to be sure.

leinardi / mypy-pycharm