SanderRonde / phpstan-vscode

PHPStan plugin for VSCode
https://marketplace.visualstudio.com/items?itemName=SanderRonde.phpstan-vscode
MIT License
47 stars 8 forks source link

[3.x] Floods CPU #71

Closed Grldk closed 7 months ago

Grldk commented 7 months ago

This is probably related to https://github.com/SanderRonde/phpstan-vscode/issues/68, and the complaints from @HomelessCoder in https://github.com/SanderRonde/phpstan-vscode/issues/70, but since upgrading to version 3 of this extension I've had to reboot several times because it completely overwhelms my system. What I think is happening is that the extension triggers checking my complete project multiple times, without waiting for the check to finish, thereby clogging up all cpu cycles. (When I search/replace in multiple files for example).

I have tried setting the max number of cpus in the phpstan config, but this only helps when I set it to a really low number, which really slows down checking the project when I want to do so manually. It seems that phpstan maximumNumberOfProcesses is applied each time phpstan starts, not to the overall number of processes, so when phpstan is triggered multiple times the number of processes is times triggered * max processes...

Changing back to singleFileMode (kindly and quickly added back, thanks!) helps, seems similar to the behaviour in v2, but does not show errors in individual files. And the scanFileForErrors command is gone in v3 as well.. So I've reverted back to v2 for now..

Not sure what the best approach to this issue is (only spawning phpstan a second time when the first one finishes if possibe?), but the default project scanning mode in v3 makes it unuseable for me unfortunately.

SanderRonde commented 7 months ago

Thanks for the report. Theoretically any running PHPStan processes should be killed before a new one starts. I'll look into whether that's happening properly.

What do you mean with "Does not show errors in individual files". And I'll add back the scanFileForErrors command.

HomelessCoder commented 7 months ago

Does not show errors in individual files

I noticed that today too. In my case, it relates to the checked file's size (or number of errors). And it works the same with 2.2.26. Is that the case for you?

What do you mean with "Does not show errors in individual files".

In my case, all reported errors (by phpstan) are in the "Problems" panel but are not inline in the editor. I don't know if that's even related to the extension or just the vscode behaviour when there are too many errors in the file... hidethepain.jpg I believe the latter. However, I discovered another nice extension usernamehw.errorlens that helped with the issue and it's actually pretty helpful :)

SanderRonde commented 7 months ago

Ahh that shouldn't be the case. So it also happens in the old version of the extension? If so then maybe not a new issue.

Could you maybe try it out on a file with very few errors? If it's not showing there then maybe something's wrong. Could be the file URL is not specified properly

HomelessCoder commented 7 months ago

Okay, I spent a bit more time on the investigation, and I think I found the root cause of the "Problems are not shown in the editor".

For some reason, the phpcs extension (I tried several different extensions) suppresses the problems reported by the intelephense and PHPStan (yours ext) from being highlighted in the editor line. However, they are in the "Problems" panel. In other words, code-style issues go over the real ones :see_no_evil: The "Error Lens" extension helps a little (it shows only one source of the problem per line, but it's still better than nothing).

Anyway, I came up with the configuration where the phpcs is disabled, but the phpcbf is triggered on-save. This ensures both error diagnosis and code formatting are in place.

Guys, I'm sorry for the off-topic. I don't know how/why it works like that, but I hope that helps someone.

SanderRonde commented 7 months ago

Ahh no worries, at least now we've figured it out :) @Grldk can you confirm that this is also the case for you?

Regarding the CPU flood, can you guys maybe tell me what platform you're on? That might help me debug.

Grldk commented 7 months ago

Thanks for the quick replies and your work on this extension in general!

Looking further into this I think the problem with errors not showing in the editor is due to how the extension works with vs code and the vs code dev container extension.

I'm using VS Code on Ubuntu (23.10). I am developing with docker, and running vs code in my docker container using the dev container extension. This worked fine with v2, but in v3 the file paths get messed up. In the problems tab I can see the errors, but the reported path is /home/{user}/Code/Laravel/{project}/app/Models/User.php, while the path should be /app/Models/User.php, as the extension is running inside the container. I can't get it to work by changing the rootDir or paths config variables either.

SanderRonde commented 7 months ago

Ah yeah that sounds like a bug in not re-applying the path-mapping. Will fix that

SanderRonde commented 7 months ago

Okay I've tackled a couple of issues:

Regarding the path mapping:

This might actually be it applying the path mapping more than it should instead of not applying it. I'm not sure I fully understand the situation here though. Are you using the paths setting? I wouldn't think you'd need to since you're running VSCode inside of the container, meaning it knows the "local" paths in the container right? Or am I missing something? Or are you using a different binary to run it?

I've created a version of the extension that guesses whether to apply path-mapping depending on whether files exist on the disk. I've attached it as a .zip file (github doesn't allow uploading of .vsix files). Can you rename it to .vsix and then install it in your editor, then tell me whether this fixes the path mapping issue? Thanks a bunch!

phpstan-vscode-3.1.0.zip

Grldk commented 7 months ago

Thanks!

phpstan-vscode-3.1.0.zip

Just tried this, it did not solve my problem. But thats because apparently I had a paths setting left in my vs code user settings that I completely missed when checking this problem first time around.. Totally my fault.. After I removed that line errors are shown as expected..

So extra path map guessing is probably not needed.. Have not spent enough time with the extension to say if the cpu flooding is fixed now.. Will report back end of day today or tomorrow.

Grldk commented 7 months ago

I've re-added the scanFileForErrors command

This doesnt work btw, using v3.1.1. It errors out with:

Please enable single-file mode in the settings to scan a single file. Instead use "Scan project for errors" to scan the whole project.

But I've enabled single file mode with "phpstan.singleFileMode": true in both my Workspace and User settings.

SanderRonde commented 7 months ago

Ah yeah I messed that one up. Fixed in 3.1.2

Grldk commented 7 months ago

Did some quick testing on CPU flooding in v3.1.2: it's not fixed unfortunately. Behaviour seems unchanged.

I can trigger a crash of my system quite reliably by search/replacing something in ~5 files. (Out of a total of little over a 1000 files that phpstan checks when checking the project) This spins of too many php process inside the docker container. Like I said before, I'm working on Ubuntu with docker. My coworker experiences the same, using vs code on windows, php in docker in wsl2. Both of us use the dev container extension so phpstan runs in the docker container.

SanderRonde commented 7 months ago

I'll give it another go then, could maybe be related to the docker container thing. Do you by any chance have a public project on which you can reproduce the issue? I don't really have a docker container PHP project readily available so it would probably take quite some time getting that set up. (but it sounds like probably not if you're talking about a work project)

Grldk commented 7 months ago

Yeah it's work, so can't share it unfortunately.

We use lando as an abstraction layer on top of docker, which makes setup really easy. Download and install docker-ce (on windows docker-desktop should work too) and lando, create a folder, add a .lando.yml with the default laravel recipe, something like this:

name: laravel
recipe: laravel
config:
  php: "8.2"
  webroot: public
  via: nginx
  cache: redis
  database: mysql:8.0
  xdebug: true

run lando start and you're good to go. You can install laravel, which is what we use, with lando composer create-project laravel/laravel

Then in vs code with the dev container extension use the attach to running container option, select the appserver container, and it should reload running in the correct container. After that run the Open Named Container Configuration File command and this json:

{
    "remoteUser": "www-data",
    "workspaceFolder": "/app"
}

to ensure vs-code runs as the right user. (I'm not quite sure this is actually needed anymore, just in case..)

SanderRonde commented 7 months ago

Thanks! I've finally managed to reproduce it and fix it :) Something was going wrong with debouncing, debouncing happened on a per-file basis but when using project-wide find+replace you'd rapidly go through a bunch of different files, each spawning their own checks and leading to race conditions that caused the killing to not work. I'm fairly confident I've actually fixed it now. Can you verify that it's fixed in the latest version? (3.1.3, should be live in a few mins)

Grldk commented 7 months ago

Just tested this a bit, seems like it works now. Cannot trigger the overload anymore. Thanks a lot!

SanderRonde commented 7 months ago

Awesome! Thanks for testing it and bearing with me until we got to the solution.