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

Freezes with 100% CPU when trying to open a specific Nix file #4862

Closed
max-privatevoid opened this issue Nov 23, 2022 · 5 comments · Fixed by #6218
Closed

Freezes with 100% CPU when trying to open a specific Nix file #4862

max-privatevoid opened this issue Nov 23, 2022 · 5 comments · Fixed by #6218
Labels
A-tree-sitter Area: Tree-sitter C-bug Category: This is a bug upstream

Comments

@max-privatevoid
Copy link
Contributor

Summary

Probably related to #2997

Reproduction Steps

  • Create a file with the content listed below
  • In another terminal, open the file in Helix
  • Observe it getting stuck before launching
  • Edit the file: Remove a single character from a string
  • Try opening the file with Helix again
  • Observe it now working even though there should be no change to the AST
{ config, lib, pkgs, ... }:
with lib;
let
  # most of the things here should probably be incorporated into a module
  
  # TODO: make a proper type for env vars or pathspecs
  coerceToEnv = val: let
    parsed = match "^\\$([a-zA-Z0-9_]*)(/(.*))?$" val;
    key = elemAt parsed 0;
    append = elemAt parsed 2;
  in if isString val && parsed != null then
      if append != null then
        concat (env key) (concat "/" (coerceToEnv append))
      else
        env key
    else if isAttrs val && val ? _sloth then
      val._sloth
    else
      val;

  instanceId = { type = "instanceId"; };
  concat = a: b: { type = "concat"; inherit a b; };
  env = key: { type = "env"; inherit key; };

  splitPath = path: if isList path then {
    a = elemAt path 0;
    b = elemAt path 1;
  } else {
    a = path;
    b = path;
  };

  bind' = arg: path: let
    split = splitPath path;
    coerced = mapAttrs (_: coerceToEnv) split;
  in [ arg coerced.a coerced.b ];
  bind = bind' "--bind-try";
  bindRo = bind' "--ro-bind-try";
  bindDev = bind' "--dev-bind-try";
  setEnv = key: val: [ "--setenv" key val ];
  mountTmpfs = path: [ "--tmpfs" path ];
  
  bindPaths = map bind config.bubblewrap.bind.rw;
  bindRoPaths = map bindRo config.bubblewrap.bind.ro;
  bindDevPaths = map bindDev config.bubblewrap.bind.dev;
  envVars = mapAttrsToList setEnv config.bubblewrap.env;
  tmpfs = map mountTmpfs config.bubblewrap.tmpfs;

  app = config.app.package;
  info = pkgs.closureInfo { rootPaths = app; };
  launcher = pkgs.callPackage ../launcher {};
  dbusOutsidePath = concat (env "XDG_RUNTIME_DIR") (concat "/nixpak-bus-" instanceId);
  
  bwrapArgs = flatten [
    "--unshare-all"
    bindPaths
    bindRoPaths
    envVars
    tmpfs
    
    (optionals config.bubblewrap.network "--share-net")
    (optionals config.bubblewrap.apivfs.dev ["--dev" "/dev"])
    (optionals config.bubblewrap.apivfs.proc ["--proc" "/proc"])

    bindDevPaths
    
    (optionals config.dbus.enable [
      (bind [ dbusOutsidePath "$XDG_RUNTIME_DIR/nixpak-bus" ])
      "--setenv" "DBUS_SESSION_BUS_ADDRESS"
      (concat "unix:path=" (coerceToEnv "$XDG_RUNTIME_DIR/nixpak-bus"))
    ])

    [ "--ro-bind" config.flatpak.infoFile "/.flatpak-info" ]

    # TODO: use closureInfo instead
    [ (bindRo "/nix/store") "${app}/${config.app.binPath}" ]
  ];
  dbusProxyArgs = [ (env "DBUS_SESSION_BUS_ADDRESS") dbusOutsidePath ] ++ config.dbus.args ++ [ "--filter" ];
  
  bwrapArgsJson = pkgs.writeText "bwrap-args.json" (builtins.toJSON bwrapArgs);
  dbusProxyArgsJson = pkgs.writeText "xdg-dbus-proxy-args.json" (builtins.toJSON dbusProxyArgs);

  mainProgram = builtins.baseNameOf config.app.binPath;

  envOverrides = pkgs.runCommand "nixpak-overrides-${app.name}" {} ''
    mkdir $out
    cd ${app}
    grep -Rl ${app}/${config.app.binPath} | xargs -r -I {} cp -r --parents {} $out || true
    find $out -type f | while read line; do
      substituteInPlace $line --replace ${app}/${config.app.binPath} ${config.script}/${config.app.binPath}
    done

    desktopFileRel=share/applications/${config.flatpak.appId}.desktop
    if [[ -e $desktopFileRel ]]; then
      echo $out
      echo -e 'nX-Flatpak=${config.flatpak.appId}' >> $out/$desktopFileRel
    fi
  '';

  # This is required because the Portal service reads /proc/$pid/root/.flatpak-info
  # from the calling PID, when dbus-proxy is in use, this PID is the dbus-proxy process
in {
}

Helix log

No response

Platform

Linux

Terminal Emulator

Tilix 1.9.5

Helix Version

helix 22.08.1

@max-privatevoid max-privatevoid added the C-bug Category: This is a bug label Nov 23, 2022
@the-mikedavis the-mikedavis added the A-tree-sitter Area: Tree-sitter label Nov 23, 2022
@the-mikedavis
Copy link
Member

the-mikedavis commented Nov 23, 2022

This is probably from the tree-sitter-bash injections within strings. Tree-sitter-bash has an external scanner that looks like other problematic scanners we've seen (#3504, #3376, #2979), and looking at the source now, it looks like there is the usual workaround in place which prevents scanning if in error recovery: https://github.com/tree-sitter/tree-sitter-bash/blob/77cf8a7cab8904baf1a721762e012644ac1d4c7b/src/scanner.cc#L171-L191. We can probably fix this by updating tree-sitter-bash

EDIT: we're already on a version of tree-sitter-bash with that scanner fix. It looks like updating tree-sitter-bash isn't enough to fix this.

@max-privatevoid
Copy link
Contributor Author

The "fix" also works in strings that are used as derivation names, or even when renaming variables. Even deleting things from the comment near the end makes the file openable. I don't think it's related to bash TS.

@the-mikedavis
Copy link
Member

the-mikedavis commented Nov 23, 2022

Looking at a flamegraph for this, it looks like we're stuck on tree-sitter's ts_parser__recover function which is the error recovery routine that caused problems in other languages. On the revision that we use (and master), tree-sitter-nix's scanner is mitigating the error recovery problem: https://github.com/cstrahan/tree-sitter-nix/blob/1b69cf1fa92366eefbe6863c184e5d2ece5f187d/src/scanner.c#L180-L190

If I comment out this block in nix injections then the problem goes away:

(apply_expression
(apply_expression
function: (apply_expression
function: ((_) @_func)))
argument: (indented_string_expression (string_fragment) @injection.content)
(#match? @_func "(^|\\.)runCommand(((No)?(CC))?(Local)?)?$")
(#set! injection.language "bash")
(#set! injection.combined))

The block I was pointing to in tree-sitter-bash isn't preventing scanning in error recovery, it's just part of the scanner 😅. Adding the usual change to the scanner that checks if we're in error recovery and returns false seems to fix this. I'll fork tree-sitter-bash and make a PR. Actually that doesn't fix it either. Something else must be keeping us in recovery mode.

Very strange that deleting from the comment removes the problem 🤔

@the-mikedavis
Copy link
Member

the-mikedavis commented Feb 6, 2023

It looks like this was a bug in tree-sitter itself and was fixed in tree-sitter/tree-sitter#1952 (also see nix-community/tree-sitter-nix#34). Running from that commit fixes this and the other linked issues for me. Once tree-sitter cuts a new version we should be able to upgrade and close this.

(If you're building from source and want to use that change before it is published and updated within Helix, you can add a patch to the Cargo.toml in the Helix repository root:)

[patch.crates-io]
tree-sitter = { git = "https://github.com/tree-sitter/tree-sitter", rev = "e021d6e9799903312dfa3a1ed39e160008fe52ac" }

@archseer
Copy link
Member

We could switch to a git dependency here too since tree-sitter hasn't been updated in 6 months. neovim did so too neovim/neovim#21858

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tree-sitter Area: Tree-sitter C-bug Category: This is a bug upstream
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants