Skip to content

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Dec 1, 2017

This commit uses the new uv_os_getpid() method to retrieve the
current process id.

I currently left GetProcessId() in util.cc. Not sure if it should be removed or not.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

src

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 1, 2017
@bnoordhuis
Copy link
Member

bnoordhuis commented Dec 1, 2017

I'd just remove GetProcessId(), it serves no purpose now.

(edit: I guess it ensures that you're always dealing with a uint32_t, which is useful in format strings.)

@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 1, 2017

I just pushed an updated commit that removes it, and just saw your edit. Should I put it back?

Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

No, I think it's fine. It's 32 bits on all our platforms so it probably makes no practical difference.

@richardlau
Copy link
Member

We should be consistent -- either it stays and the rest of the code base uses it or it is removed and the code base directly calls uv_os_getpid().

@Fishrock123
Copy link
Contributor

Can uv_os_getpid() fail? If not it doesn't matter but if it can I guess we could fall back to that stuff?

@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 1, 2017

It cannot.

@bnoordhuis
Copy link
Member

getpid() can fail on Linux in a seccomp2 sandbox. The sandbox can reject the system call with an ENOSYS error, for example.

But that's probably an academical concern, and arguably a bug in the sandbox, not the application.

@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 2, 2017

TIL. We also wouldn't use the old code as a fallback since uv_os_getpid() is built using it 😄

This commit uses the new uv_os_getpid() method to retrieve the
current process id.

PR-URL: nodejs#17415
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
@cjihrig
Copy link
Contributor Author

cjihrig commented Dec 4, 2017

@cjihrig cjihrig merged commit a803bca into nodejs:master Dec 4, 2017
@cjihrig cjihrig deleted the getpid branch December 4, 2017 15:38
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
This commit uses the new uv_os_getpid() method to retrieve the
current process id.

PR-URL: #17415
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
MylesBorins pushed a commit that referenced this pull request Dec 12, 2017
This commit uses the new uv_os_getpid() method to retrieve the
current process id.

PR-URL: #17415
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Dec 12, 2017
@gibfahn
Copy link
Member

gibfahn commented Dec 20, 2017

Should be good to land in the next 6.x and 8.x releases.

@richardlau
Copy link
Member

@gibfahn FYI this PR depends on libuv 1.18.0.

@MylesBorins
Copy link
Contributor

@richardlau I think we'll be updating libuv in the next version (as long as it doesn't change gcc requirements)

gibfahn pushed a commit that referenced this pull request Feb 19, 2018
This commit uses the new uv_os_getpid() method to retrieve the
current process id.

PR-URL: #17415
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
@MylesBorins
Copy link
Contributor

6.x is currently running on libuv 1.16.1, so I'm setting this to don't-land

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.