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

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

Merged
merged 1 commit into from
Aug 22, 2018
Merged

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

merged 1 commit into from
Aug 22, 2018

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Aug 20, 2018

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 nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Aug 20, 2018
@refack
Copy link
Contributor

refack commented Aug 20, 2018

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

@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 20, 2018

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

  1. The normal argument about not continuing to add everything to process.
  2. 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
Copy link
Contributor

refack commented Aug 20, 2018

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 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
@refack
Copy link
Contributor

refack commented Aug 20, 2018

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)
  ...

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

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

doc/api/os.md Outdated
* `pid` {integer} The process ID to set scheduling priority for.
**Default** `0`.
* `priority` {integer} The scheduling priority to assign to the process.
* Returns: {undefined}
Copy link
Contributor

Choose a reason for hiding this comment

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

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

doc/api/os.md Outdated
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

doc/api/os.md Outdated
<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
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@cjihrig cjihrig force-pushed the setpriority branch 2 times, most recently from 1d22722 to 1e6f131 Compare August 22, 2018 17:02
PR-URL: nodejs#22407
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@cjihrig
Copy link
Contributor Author

cjihrig commented Aug 22, 2018

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

@cjihrig cjihrig merged commit 92880f3 into nodejs:master Aug 22, 2018
@cjihrig cjihrig deleted the setpriority branch August 22, 2018 21:29
targos pushed a commit that referenced this pull request Aug 24, 2018
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
PR-URL: #22407
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
targos added a commit that referenced this pull request Sep 6, 2018
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
targos added a commit that referenced this pull request Sep 6, 2018
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
targos added a commit that referenced this pull request Sep 6, 2018
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
@Fishrock123 Fishrock123 mentioned this pull request Sep 21, 2018
4 tasks
@Unitech Unitech mentioned this pull request Oct 5, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. libuv Issues and PRs related to the libuv dependency or the uv binding. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants