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

src: use GetCurrentProcessId() for process.pid #4163

Merged
merged 1 commit into from
Dec 5, 2015

Conversation

bnoordhuis
Copy link
Member

Commit a9c0c65 ("src: define getpid() based on OS") made src/env.cc
use GetCurrentProcessId() on Windows for the PID in log messages.
GetCurrentProcessId() is also what is used by libuv, OpenSSL and V8.

This commit makes process.pid use GetCurrentProcessId() instead of
_getpid() for consistency.

R=@cjihrig?

CI: https://ci.nodejs.org/job/node-test-pull-request/930/

@bnoordhuis bnoordhuis added the windows Issues and PRs related to the Windows platform. label Dec 5, 2015
@bnoordhuis
Copy link
Member Author

Refs #4146.

@rvagg
Copy link
Member

rvagg commented Dec 5, 2015

lgtm pending CI, +1 for consistency even though I can't find any information about _getpid vs GetProcessId

@bnoordhuis
Copy link
Member Author

There are failures on the Windows buildbots but they are all known flakes (#3956, #3957, #4057).

@cjihrig
Copy link
Contributor

cjihrig commented Dec 5, 2015

LGTM

@mscdex
Copy link
Contributor

mscdex commented Dec 5, 2015

LGTM.

@rvagg I believe _getpid() calls GetCurrentProcessId() internally.

@mscdex mscdex added the lib / src Issues and PRs related to general changes in the lib or src directory. label Dec 5, 2015
Commit a9c0c65 ("src: define getpid() based on OS") made src/env.cc
use `GetCurrentProcessId()` on Windows for the PID in log messages.
`GetCurrentProcessId()` is also what is used by libuv, OpenSSL and V8.

This commit makes `process.pid` use `GetCurrentProcessId()` instead of
`_getpid()` for consistency.

PR-URL: nodejs#4163
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
@bnoordhuis bnoordhuis closed this Dec 5, 2015
@bnoordhuis bnoordhuis deleted the process-pid-windows branch December 5, 2015 16:14
@bnoordhuis bnoordhuis merged commit e6e7891 into nodejs:master Dec 5, 2015
bnoordhuis added a commit that referenced this pull request Dec 8, 2015
Commit a9c0c65 ("src: define getpid() based on OS") made src/env.cc
use `GetCurrentProcessId()` on Windows for the PID in log messages.
`GetCurrentProcessId()` is also what is used by libuv, OpenSSL and V8.

This commit makes `process.pid` use `GetCurrentProcessId()` instead of
`_getpid()` for consistency.

PR-URL: #4163
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
@rvagg rvagg mentioned this pull request Dec 17, 2015
bnoordhuis added a commit that referenced this pull request Dec 29, 2015
Commit a9c0c65 ("src: define getpid() based on OS") made src/env.cc
use `GetCurrentProcessId()` on Windows for the PID in log messages.
`GetCurrentProcessId()` is also what is used by libuv, OpenSSL and V8.

This commit makes `process.pid` use `GetCurrentProcessId()` instead of
`_getpid()` for consistency.

PR-URL: #4163
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
MylesBorins pushed a commit that referenced this pull request Jan 19, 2016
Commit a9c0c65 ("src: define getpid() based on OS") made src/env.cc
use `GetCurrentProcessId()` on Windows for the PID in log messages.
`GetCurrentProcessId()` is also what is used by libuv, OpenSSL and V8.

This commit makes `process.pid` use `GetCurrentProcessId()` instead of
`_getpid()` for consistency.

PR-URL: #4163
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
@MylesBorins MylesBorins mentioned this pull request Jan 19, 2016
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
Commit a9c0c65 ("src: define getpid() based on OS") made src/env.cc
use `GetCurrentProcessId()` on Windows for the PID in log messages.
`GetCurrentProcessId()` is also what is used by libuv, OpenSSL and V8.

This commit makes `process.pid` use `GetCurrentProcessId()` instead of
`_getpid()` for consistency.

PR-URL: nodejs#4163
Reviewed-By: Brian White <mscdex@mscdex.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Rod Vagg <rod@vagg.org>
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. windows Issues and PRs related to the Windows platform.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants