os: add os.{get,set}Priority() #22407

cjihrig · 2018-08-20T00:57:52Z

Allow Node to set and set process priorities. ~~This PR requires #22365 (libuv update) and #22394 (bug fix in os module). Ignore the first commit, which is just the libuv update from #22365.~~

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

nodejs-github-bot · 2018-08-20T00:57:53Z

@cjihrig build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/598/pipeline

refack · 2018-08-20T13:14:29Z

_{A touch of bikeshedding.}
Seems classic to me to add it to process to get process.setPriority(gaga)

cjihrig · 2018-08-20T13:18:27Z

My two cents on why I don't think we should add it to process:

The normal argument about not continuing to add everything to process.
Since you can get/set the priority of processes other than the current one, I think it makes sense to leave it off of process.

refack · 2018-08-20T13:26:38Z

Another out-of-the-box thought. Alias the constants on the JS function object to get:

const { setPriority } = require('os');
setPriority(111, setPriority.PRIORITY_LOW);

It's non convention but might be more usable.
_{@cjihrig feel free to ignore me. just leaving ideas}

refack · 2018-08-20T13:30:13Z

Ohh found a real comment, need to fix parallel/test-binding-constants

not ok 149 parallel/test-binding-constants
  ---
  duration_ms: 0.138
  severity: fail
  exitcode: 1
  stack: |-
    assert.js:84
      throw new AssertionError(obj);
      ^
    
    AssertionError [ERR_ASSERTION]: Expected inputs to be strictly deep-equal:
    + actual - expected
    
      [
        'UV_UDP_REUSEADDR',
        'dlopen',
        'errno',
    +   'priority',
        'signals'
      ]
        at Object.<anonymous> (/home/travis/build/nodejs/node/test/parallel/test-binding-constants.js:11:8)
        at Module._compile (internal/modules/cjs/loader.js:689:30)
        at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
        at Module.load (internal/modules/cjs/loader.js:599:32)
        at tryModuleLoad (internal/modules/cjs/loader.js:538:12)
        at Function.Module._load (internal/modules/cjs/loader.js:530:3)
        at Function.Module.runMain (internal/modules/cjs/loader.js:742:12)
        at startup (internal/bootstrap/node.js:257:19)
        at bootstrapNodeJSCore (internal/bootstrap/node.js:634:3)
  ...

targos · 2018-08-20T13:54:19Z

test/parallel/test-os-process-priority.js

+    os.setPriority(0, priority);
+  }, {
+    code: 'ERR_SYSTEM_ERROR',
+    message: /A system error occurred: uv_os_setpriority returned EINVAL/


I feel like it would be better for our users if we checked the range in JS. We could throw a more specific RangeError.

Agreed. A specific RangeError with a friendlier error message would be good.

vsemozhetbyt · 2018-08-20T16:58:03Z

doc/api/os.md

+* `pid` {integer} The process ID to set scheduling priority for.
+  **Default** `0`.
+* `priority` {integer} The scheduling priority to assign to the process.
+* Returns: {undefined}


Nit: it seems we usually just omit this line if a method returns undefined.

vsemozhetbyt · 2018-08-20T16:59:43Z

doc/api/os.md

+process specified by `pid`. If `pid` is not provided, or is `0`, the priority
+of the current process is used.
+
+The `priority` input must be an integer between -20 (high priority) and 19 (low


-20 -> `-20`, 19 -> `19`?

vsemozhetbyt · 2018-08-20T17:05:54Z

doc/api/os.md

+  <tr>
+    <td><code>PRIORITY_LOW</code></td>
+    <td>The lowest process scheduling priority. This corresponds to
+    `IDLE_PRIORITY_CLASS` on Windows, and a nice value of `19` on all other


If I am not mistaken. markdown is not processed inside HTML tags, so we need <code> elements instead of backticks in this table.

PR-URL: nodejs#22407 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

cjihrig · 2018-08-22T21:28:56Z

Nits addressed. Green CI: https://ci.nodejs.org/job/node-test-pull-request/16684/

PR-URL: #22407 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

Notable changes: * child_process: * `TypedArray` and `DataView` values are now accepted as input by `execFileSync` and `spawnSync`. #22409 * coverage: * Native V8 code coverage information can now be output to disk by setting the environment variable `NODE_V8_COVERAGE` to a directory. #22527 * deps: * The bundled npm was upgraded to version 6.4.1. #22591 * Changelogs: [6.3.0-next.0](https://github.com/npm/cli/releases/tag/v6.3.0-next.0) [6.3.0](https://github.com/npm/cli/releases/tag/v6.3.0) [6.4.0](https://github.com/npm/cli/releases/tag/v6.4.0) [6.4.1](https://github.com/npm/cli/releases/tag/v6.4.1) * fs: * The methods `fs.read`, `fs.readSync`, `fs.write`, `fs.writeSync`, `fs.writeFile` and `fs.writeFileSync` now all accept `TypedArray` and `DataView` objects. #22150 * A new boolean option, `withFileTypes`, can be passed to to `fs.readdir` and `fs.readdirSync`. If set to true, the methods return an array of directory entries. These are objects that can be used to determine the type of each entry and filter them based on that without calling `fs.stat`. #22020 * http2: * The `http2` module is no longer experimental. #22466 * os: * Added two new methods: `os.getPriority` and `os.setPriority`, allowing to manipulate the scheduling priority of processes. #22407 * process: * Added `process.allowedNodeEnvironmentFlags`. This object can be used to programmatically validate and list flags that are allowed in the `NODE_OPTIONS` environment variable. #19335 * src: * Deprecated option variables in public C++ API. #22515 * Refactored options parsing. #22392 * vm: * Added `vm.compileFunction`, a method to create new JavaScript functions from a source body, with options similar to those of the other `vm` methods. #21571 * Added new collaborators: * [lundibundi](https://github.com/lundibundi) - Denys Otrishko PR-URL: #22716

nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 20, 2018

jasnell approved these changes Aug 20, 2018

View reviewed changes

refack approved these changes Aug 20, 2018

View reviewed changes

refack added os Issues and PRs related to the os subsystem. semver-minor PRs that contain new features and should be released in the next minor version. libuv Issues and PRs related to the libuv dependency or the uv binding. labels Aug 20, 2018

targos reviewed Aug 20, 2018

View reviewed changes

vsemozhetbyt reviewed Aug 20, 2018

View reviewed changes

cjihrig force-pushed the setpriority branch 2 times, most recently from 1d22722 to 1e6f131 Compare August 22, 2018 17:02

os: add os.{get,set}Priority()

92880f3

PR-URL: nodejs#22407 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

cjihrig force-pushed the setpriority branch from 1e6f131 to 92880f3 Compare August 22, 2018 21:28

cjihrig merged commit 92880f3 into nodejs:master Aug 22, 2018

cjihrig deleted the setpriority branch August 22, 2018 21:29

targos pushed a commit that referenced this pull request Aug 24, 2018

os: add os.{get,set}Priority()

184cd65

PR-URL: #22407 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

targos pushed a commit that referenced this pull request Sep 3, 2018

os: add os.{get,set}Priority()

219da67

PR-URL: #22407 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com>

targos mentioned this pull request Sep 5, 2018

Release proposal: v10.10.0 #22716

Merged

This was referenced Sep 6, 2018

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-android-nodejs#236

Closed

2018-09-06 Version 10.10.0 (Current) @targos beevelop/docker-nodejs#221

Closed

kfarnung mentioned this pull request Sep 10, 2018

test-os-process-priority failing on my Windows machine #22799

Closed

Fishrock123 mentioned this pull request Sep 21, 2018

process: add nice() function #21675

Closed

4 tasks

Unitech mentioned this pull request Oct 5, 2018

Cpu limit Unitech/pm2#3912

Closed

snyk-bot mentioned this pull request Aug 15, 2021

[Snyk] Fix for 1 vulnerabilities ron-a20/node#39

Open

molllyn1 mentioned this pull request Mar 22, 2024

[Snyk] Fix for 2 vulnerabilities molllyn1/node#106

Open

137717unity mentioned this pull request Mar 22, 2024

[Snyk] Fix for 2 vulnerabilities 137717unity/node#122

Open

kissofire mentioned this pull request Mar 22, 2024

[Snyk] Fix for 2 vulnerabilities kissofire/node#124

Open

aliceUnhinged613 mentioned this pull request Mar 22, 2024

[Snyk] Fix for 2 vulnerabilities aliceUnhinged613/node#123

Open

ron-a20 mentioned this pull request Mar 22, 2024

[Snyk] Fix for 2 vulnerabilities ron-a20/node#121

Open

wedataintelligence mentioned this pull request Mar 23, 2024

[Snyk] Fix for 2 vulnerabilities wedataintelligence/node#105

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

os: add os.{get,set}Priority() #22407

os: add os.{get,set}Priority() #22407

cjihrig commented Aug 20, 2018 •

edited

Loading

nodejs-github-bot commented Aug 20, 2018

refack commented Aug 20, 2018

cjihrig commented Aug 20, 2018

refack commented Aug 20, 2018

refack commented Aug 20, 2018

targos Aug 20, 2018

jasnell Aug 20, 2018

vsemozhetbyt Aug 20, 2018

vsemozhetbyt Aug 20, 2018

vsemozhetbyt Aug 20, 2018

cjihrig commented Aug 22, 2018

os: add os.{get,set}Priority() #22407

os: add os.{get,set}Priority() #22407

Conversation

cjihrig commented Aug 20, 2018 • edited Loading

Checklist

nodejs-github-bot commented Aug 20, 2018

refack commented Aug 20, 2018

cjihrig commented Aug 20, 2018

refack commented Aug 20, 2018

refack commented Aug 20, 2018

targos Aug 20, 2018

Choose a reason for hiding this comment

jasnell Aug 20, 2018

Choose a reason for hiding this comment

vsemozhetbyt Aug 20, 2018

Choose a reason for hiding this comment

vsemozhetbyt Aug 20, 2018

Choose a reason for hiding this comment

vsemozhetbyt Aug 20, 2018

Choose a reason for hiding this comment

cjihrig commented Aug 22, 2018

cjihrig commented Aug 20, 2018 •

edited

Loading