Skip to content

unix: handle EINTR in uv_resident_set_memory on AIX and SunOS#5034

Open
Nandann018-ux wants to merge 2 commits intolibuv:v1.xfrom
Nandann018-ux:v1.x
Open

unix: handle EINTR in uv_resident_set_memory on AIX and SunOS#5034
Nandann018-ux wants to merge 2 commits intolibuv:v1.xfrom
Nandann018-ux:v1.x

Conversation

@Nandann018-ux
Copy link
Copy Markdown

Summary

This PR improves the robustness of uv_resident_set_memory on AIX and SunOS by properly handling EINTR from read(2).

On these platforms, read(2) can be interrupted by a signal and return -1 with errno == EINTR. Previously, this condition was treated as a generic failure and mapped to UV_EINVAL without retrying. As a result, legitimate interruptions could surface as misleading errors and cause spurious failures.

Problem

  • read(2) may fail with EINTR when interrupted by a signal.
  • The existing implementation did not retry the call.
  • The function returned UV_EINVAL, masking the real cause of the failure.
  • This could lead to incorrect error reporting and unnecessary test or runtime failures.

Solution

  • Retry read(2) when it fails with EINTR.
  • Preserve and return accurate error codes for genuine failures.
  • Maintain unchanged behavior for successful execution paths.

Impact

  • Improves correctness and reliability on AIX and SunOS.
  • Prevents spurious failures caused by signal interruptions.
  • No behavioral changes in successful scenarios.
  • No API changes.

Platforms Affected

  • AIX
  • SunOS

Copy link
Copy Markdown
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

I don't know if procfs can return EINTR, but this looks correct for handling it anyways

@vtjnash
Copy link
Copy Markdown
Member

vtjnash commented Feb 22, 2026

Out of curiosity, which AI did you use to find this and make the PR?

@Nandann018-ux
Copy link
Copy Markdown
Author

@vtjnash Thanks for taking a look!
I used ChatGPT (codex) to help reason about EINTR handling patterns, but I verified the behavior manually and made sure the change matches libuv’s existing error-handling style and platform constraints.

@Nandann018-ux
Copy link
Copy Markdown
Author

@vtjnash
Please let me know if you’d like any additional changes — I’m happy to update the PR.

@Nandann018-ux
Copy link
Copy Markdown
Author

Nandann018-ux commented Mar 2, 2026

@japj Please review my pr and would like to change if there are any further changes.

@japj
Copy link
Copy Markdown
Contributor

japj commented Mar 2, 2026

@Nandann018-ux please follow the procedure at https://github.com/libuv/libuv/blob/v1.x/CONTRIBUTING.md

Nowhere does it mention that you should tag specific users.
As mentioned “Pull requests are usually reviewed within a few days”, there is no need to try to “bump the priority” of this PR.

I don’t know how you “selected” the users to ping, but they are definitely not current maintainers of this project, see also https://github.com/libuv/libuv/blob/v1.x/MAINTAINERS.md (but please do not ping them instead).

If you need to contact someone from the project for a discussion on how to best contribute, please use the existing community links at https://github.com/libuv/libuv/tree/v1.x?tab=readme-ov-file#community

@Nandann018-ux
Copy link
Copy Markdown
Author

@japj
Thanks for the guidance-understood. I’ve removed the pings and will follow CONTRIBUTING going forward. I won’t tag maintainers or try to bump priority; I’ll use the community links if I need discussion. Appreciate the clarification.

@Nandann018-ux
Copy link
Copy Markdown
Author

@japj Can you please re-review this PR and get it merged?

@japj
Copy link
Copy Markdown
Contributor

japj commented Mar 27, 2026

@japj Can you please re-review this PR and get it merged?

No, I cannot. I am not a maintainer of this project as you know from reading the contributor and maintainer documentation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants