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

Improve the efficiency and robustness of win32_getppid #116195

Closed
vxiiduu opened this issue Mar 1, 2024 · 4 comments
Closed

Improve the efficiency and robustness of win32_getppid #116195

vxiiduu opened this issue Mar 1, 2024 · 4 comments
Labels
OS-windows type-feature A feature request or enhancement

Comments

@vxiiduu
Copy link
Contributor

vxiiduu commented Mar 1, 2024

Feature or enhancement

Proposal:

I have made a pull request #116205. The current implementation uses more resources and more failure-prone than necessary, so I have made an enhancement which does not affect existing code.

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

Links to previous discussion of this feature:

No response

Linked PRs

@zooba
Copy link
Member

zooba commented Mar 1, 2024

I appreciate the improvement, but we have a policy of not using ntdll functions in CPython (yes, these ones are documented, but they're documented explicitly as "use alternatives").

Have you observed failures due to the current implementation? Heap allocation shouldn't be a problem - if it is then you're about to have way more problems anyway - but I can definitely see that it's inefficient. Intermittent failures would be a real problem

Performance alone isn't critical on this function - the value cannot change, so nobody should ever need to call it more than once (and we could cache the result). Using the correct documented APIs is preferable to just speed.

Overall, if the change handles failure in the unsupported API, including handling blatantly invalid looking process ID values (e.g. >32-bits, zero, -1, etc) and we fall back to the supported API, I'm probably okay with it. But that really doesn't help maintainability, so we'd want there to be a reliability concern to justify it.

@vxiiduu
Copy link
Contributor Author

vxiiduu commented Mar 1, 2024

I appreciate the improvement, but we have a policy of not using ntdll functions in CPython (yes, these ones are documented, but they're documented explicitly as "use alternatives").

Given the fact that this function and the way I've called it is documented and has been documented for over 10 years (not sure on the exact date when they began to document it), plus the fact that the function itself and the info class (ProcessBasicInformation) along with its accompanying structure has remained exactly identical and unchanged in terms of API for every single version of NT-based Windows in existence (for over 30 years), I would say that the chance of Microsoft making a breaking change is about nothing. Changing the behavior of this call scenario would break huge amounts of existing software and drivers for no tangible benefit to Microsoft.

Have you observed failures due to the current implementation?

I haven't. I came across this piece of code in Python by doing an API trace as a part of an unrelated project and after noticing that this entire class of process snapshot APIs was being used in a rather roundabout way I just decided to write a replacement function - given that there is a direct, 1-step method to get the same result (similar to the linux syscall getppid).

Performance alone isn't critical on this function - the value cannot change, so nobody should ever need to call it more than once (and we could cache the result). Using the correct documented APIs is preferable to just speed.
Overall, if the change handles failure in the unsupported API, including handling blatantly invalid looking process ID values (e.g. >32-bits, zero, -1, etc) and we fall back to the supported API, I'm probably okay with it. But that really doesn't help maintainability, so we'd want there to be a reliability concern to justify it.

If the usage of a NTDLL function doesn't outright disqualify this pull request from being merged (not quite sure what your stance is on that) then I'd be happy to revise the code to cache the PID and/or put the old code back in as a fallback. Let me know.

@zooba
Copy link
Member

zooba commented Mar 1, 2024

If the usage of a NTDLL function doesn't outright disqualify this pull request from being merged (not quite sure what your stance is on that) then I'd be happy to revise the code to cache the PID and/or put the old code back in as a fallback.

Technically it does, but because I agree this is a very unlikely API to ever change or break, I'm prepared to overrule the rule.

But because I know how people work, I don't want the next contributor to point at this case and argue that the rule clearly doesn't exist.

So if you're happy to implement a win32_getppid_fast function and fall back to the current implementation if it fails/looks wrong, I'm happy to take it.

zooba pushed a commit that referenced this issue Mar 14, 2024
Use the NtQueryInformationProcess system call to efficiently retrieve the parent process ID in a single step, rather than using the process snapshots API which retrieves large amounts of unnecessary information and is more prone to failure (since it makes heap allocations).

Includes a fallback to the original win32_getppid implementation in case the unstable API appears to return strange results.
@zooba
Copy link
Member

zooba commented Mar 14, 2024

Thanks for your work on this!

@zooba zooba closed this as completed Mar 14, 2024
vstinner pushed a commit to vstinner/cpython that referenced this issue Mar 20, 2024
Use the NtQueryInformationProcess system call to efficiently retrieve the parent process ID in a single step, rather than using the process snapshots API which retrieves large amounts of unnecessary information and is more prone to failure (since it makes heap allocations).

Includes a fallback to the original win32_getppid implementation in case the unstable API appears to return strange results.
adorilson pushed a commit to adorilson/cpython that referenced this issue Mar 25, 2024
Use the NtQueryInformationProcess system call to efficiently retrieve the parent process ID in a single step, rather than using the process snapshots API which retrieves large amounts of unnecessary information and is more prone to failure (since it makes heap allocations).

Includes a fallback to the original win32_getppid implementation in case the unstable API appears to return strange results.
diegorusso pushed a commit to diegorusso/cpython that referenced this issue Apr 17, 2024
Use the NtQueryInformationProcess system call to efficiently retrieve the parent process ID in a single step, rather than using the process snapshots API which retrieves large amounts of unnecessary information and is more prone to failure (since it makes heap allocations).

Includes a fallback to the original win32_getppid implementation in case the unstable API appears to return strange results.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OS-windows type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

3 participants