Skip to content
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

Crash when removing folders in sublime text #2516

Closed
gerardroche opened this issue Jan 23, 2024 · 5 comments
Closed

Crash when removing folders in sublime text #2516

gerardroche opened this issue Jan 23, 2024 · 5 comments

Comments

@gerardroche
Copy link
Contributor

gerardroche commented Jan 23, 2024

phpactor crashes including crashing other things like firefox, terminal, and also gnome shell itself. Obviously this is quiet a severe crash.

I'm using phpactor in Sublime Text LSP and can consistently reproduce the crash when removing several folders in quick succession. It's not the only context in which the crash occurs, but I can reproduce that one.

I reported the issue in the Sublime text LSP plugin, but I've narrowed the issue down to phpactor. There are additional details on the issue, but I'll add some here.

My Sublime Text lsp config, the configurations outside of "clients" don't seem to be relevant, but I'll show anyway:

{

    "diagnostics_delay_ms": 50,
    "diagnostics_gutter_marker": "dot",
    "log_debug": false,
    "log_server": false,
    "show_code_actions": "",
    "show_diagnostics_count_in_view_status": false,
    "show_diagnostics_in_view_status": false,
    "show_inlay_hints": false,
    "show_multiline_diagnostics_highlights": false,
    "show_view_status": false,

    "diagnostics_highlight_style": {
        "error": "",
        "warning": "",
        "info": "stippled",
        "hint": "stippled"
    },

    "clients": {
        "phpactor": {
            "command": [
                "php",
                "-d",
                "xdebug.mode=off",
                "/usr/local/bin/phpactor",
                "language-server",
            ],
            "enabled": true,
            "languageId": "php",
            "scopes": [
                "source.php",
                "embedding.php"
            ],
            "syntaxes": [
                "Packages/PHP/PHP.sublime-syntax"
            ],
        },
    },
}

I don't think it probably matters how I have it installed, but I have phpactor installed via composer. The /usr/local/bin/phpactor is symlinked to bin/phpactor in my cloned phpactor repository which is installed, for example, like this':

cd ~
git clone https://github.com/phpactor/phpactor
cd phpactor
composer install --no-dev --optimize-autoloader
cd /usr/local/bin
sudo ln -s ~/phpactor/phpactor/bin/phpactor

To reproduce the issue

  1. Add several project folders to sublime
  2. Open a php file (opening a php file initialises the phpactor language-server).
  3. Remove two of the folders one after the other quickly (less than about 1 sec apart)

I use my Sesame plugin so it's easy to remove folders quickly, but you can do it without Sesame, you just need to be quick with you mouse.

I enabled phpactor logging but there is nothing of note in the log.

Other notes

  • Sublime 4169
  • Ubuntu 22.04
  • Gnome Shell 42.9
  • Windowing System Wayland

In Sublime the code to remove a folder is:

def _remove_folder(window: Window, folder: str) -> None:
    window.run_command('remove_folder', {
        'dirs': [folder]
    })

So you should be able to run the command via the console if you like:

>>> sublime.active_window().run_command('remove_folder', {'dirs': ['/path/to/folder']})

Otherwise right click the folder in side bar and select Remove Folder from Project.

@gerardroche
Copy link
Contributor Author

See sublimelsp/LSP#2380 (comment)

I think it's this:

https://github.com/amphp/process/blob/v1.1.4/lib/Internal/Posix/Runner.php#L197

@\posix_kill($pid, 9);

When there is an error, $pid is null and this causes everything to crash. When add a null check, the problem seems to go away.

The php docs says nothing about null, but I think a null would be coerced to 0 so posix_kill() would receive 0 in the case of an error.

php > var_dump((int) null);
int(0)

phpactor is using an old version of amphp/process: v1.1.4 - the newest version is v2.0.1 so maybe it's fixed. The newer code looks a lot better.

gerardroche added a commit to gerardroche/process that referenced this issue Jan 25, 2024
When `$pid` in `posix_kill($pid, $signo)` is `null` it kills other
programs like Firefox, Terminal, even Gnome Shell. This patch adds a
null check to avoid this.

Discovered via phpactor in Sublime Text: removing several folders in
quick succession causes `posix_kill` to be called with `null`, for
whatever reason, which causes a bunch of programs to crash.

See phpactor/phpactor#2516
@gerardroche
Copy link
Contributor Author

This is an issue in amphp/process dependency. I've sent a patch:

amphp/process#68

When it's merged, the dependency can be updated to get the patch.

@dantleech
Copy link
Collaborator

Neovim sometimes crashes when deleting folders, I wonder if it can be related.

Upgrading to Amp 2.0 would be great in general, but also they switch to fibers, so not sure how much work it would be.

The only place in the code I can find that directly invokes kill is

$process->kill();
which only gets called when killing Psalm or the Phpactor diagnostics process after a timeout.

Did you identify the stack trace for when this happens in Sublime?

@gerardroche
Copy link
Contributor Author

I confirmed that it was the posix_kill() calls in https://github.com/amphp/process/pull/68/files. Essentially, if the pid is null, posix_kill() kills most open programs. I think it might be technically something like, all programs in the same process group, or sibling processes. Either way, it kills a lot of programs.

I could never discover why the pid is null. I assume there is an error somewhere, but there is no exception. I didn't get around to trying to use debugger to see what is happening.

I also attempted to upgrade to Amp 2.0, but it's a little more complicated than I anticipated, so backed out of it. The issue might be fixed in v2. I seen related commits in the git log. Even if the issue is still v2, it's an easy null-check fix.

kelunik added a commit to amphp/process that referenced this issue Feb 24, 2024
When `$pid` in `posix_kill($pid, $signo)` is `null` it kills other programs like Firefox, Terminal, even Gnome Shell.

This patch adds a null check to avoid this.

Discovered via phpactor in Sublime Text: Removing several folders in quick succession causes `posix_kill` to be called with `null`, for whatever reason, which causes a bunch of programs to crash.

See phpactor/phpactor#2516.

Co-authored-by: Niklas Keller <me@kelunik.com>
@dantleech
Copy link
Collaborator

dantleech commented Feb 24, 2024

The PR has been merged and tagged! - can you confirm the fix #2560 ?

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

No branches or pull requests

2 participants