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

Fix for Critical Issue #2: Spawning 100% CPU Processes that Never Exit #3

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

PowerWeb5
Copy link

@PowerWeb5 PowerWeb5 commented Jun 6, 2019

Fix for critical bug #2 "100% CPU" and related enhancements

Fixes & Enhancements

  • Fix for critical bug 100% CPU #2 "100% CPU" - No longer spawns many node.exe processes which use 100% of a CPU core each and never terminate, even days later.
  • Added options (which can change hard-coded or expose as user settings ideally) for reducing CPU priority
  • Git ignored package-lock.json
  • Changed extension category to new "Programming Languages" instead of "Programming" to fix warning for that
  • Updated version, bumped to 1.0.0 since is fairly stable and unlikely to change much further soon.
  • Mentioned TypeScript in extension/package description as well.
  • Detailed comments added and commented out code can use if needed, as well as comments for suggested improvements

Notes

  • I added a dependency package, so be sure to run npm install after pulling

To-Do Suggestions

  • Needs further testing first to ensure doesn't break spawning the node.exe for nak file search process and that always exits even if Ctrl-Click / Go to Definition for If or Var keywords in large projects with many node modules, for example.
  • Expose settings under //Settings: section in search.js as extension settings to control timeout, CPU priority, etc.

…h use 100% of a CPU core each and never terminate, even days later.

Added options (which can change hard-coded or expose as user settings ideally) for reducing CPU priority
Note: After pull, run npm install to install new dependencies added to package.json
Git ignored package-lock.json
Changed extension category to new "Programming Languages" instead of "Programming" to fix warning for that
TODO: Expose settings under //Settings: section in search.js as extension settings to control timeout, CPU priority, etc.
TODO: Requires further testing to ensure doesn't break spawning the node.exe for nak file search proccess and that always exits even if Ctrl-Click / Go to Definition for If or Var keywords in large projects with many node modules, for example.
…to change much further soon.

Mentioned TypeScript in extension/package description as well.
@PowerWeb5 PowerWeb5 mentioned this pull request Jun 6, 2019
Copy link
Owner

@jrieken jrieken left a comment

Choose a reason for hiding this comment

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

This seems like a largish change and it seems to contain changes that aren't necessary. To fix this issue, isn't it enough to invoke kill with SIGKILL?

@@ -1,9 +1,9 @@
{
"name": "fuzzy-definitions",
"displayName": "Fuzzy Definitions",
"description": "Fuzzy 'Go to Definition' for JavaScript",
"description": "Fuzzy 'Go to Definition' for JavaScript and TypeScript",
Copy link
Owner

Choose a reason for hiding this comment

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

and TypeScript

@@ -19,7 +19,7 @@
"vscode": "^1.0.0"
},
"categories": [
"Languages"
"Programming Languages"
Copy link
Owner

Choose a reason for hiding this comment

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

Unrelated change?

@@ -3,7 +3,8 @@
import * as vscode from 'vscode';
import {extname, dirname, join} from 'path';
import {exec} from 'child_process';

//import {which} from 'shelljs';
let which = require('shelljs').which;
Copy link
Owner

Choose a reason for hiding this comment

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

why not from?

const nak = exec(cmd, (err, stdout, stderr) => {

// Settings: TODO: Expose below extension settings to users (possibly pattern for advanced users too?)

Copy link
Owner

Choose a reason for hiding this comment

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

Better not - once we have microsoft/vscode#73524 this extension can re-use the VS Code search infrastructure.

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

Successfully merging this pull request may close these issues.

2 participants