Skip to content

Conversation

hamarb123
Copy link
Contributor

Fixes #117299.
Affects the following APIs: #117299 (comment).

@dotnet-policy-service dotnet-policy-service bot added the community-contribution Indicates that the PR has been added by a community member label Jul 5, 2025
@github-actions github-actions bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 5, 2025
@hamarb123
Copy link
Contributor Author

hamarb123 commented Jul 5, 2025

Note: there's one place where we're not currently treating EINTR from close as don't repeat that we may want to change:

And we could also optionally mark EINTR on close as success in a few places on apple platforms now (e.g., SystemNative_Close) (and also linux according to https://issues.chromium.org/issues/41033222 (source from linus), as it has this behaviour built into normal close), as the close$NOCANCEL version guarantees it isn't leaked when EINTR is returned.

Please lmk what I should do with these.

@jkotas
Copy link
Member

jkotas commented Jul 5, 2025

there's one place where we're not currently treating EINTR from close as don't repeat that we may want to change:

I think so.

And we could also optionally mark EINTR on close as success in a few places on apple platforms now

Is there any place where it makes a difference? We seem to be ignoring EINTR returned by close everywhere.

I have noticed that t_lastCloseErrorInfo in SafeFileHandle.Unix.cs looks like a dead code. I think it can be deleted.

@hamarb123
Copy link
Contributor Author

hamarb123 commented Jul 6, 2025

Is there any place where it makes a difference? We seem to be ignoring EINTR returned by close everywhere.

Btw, there's 2 places where we're already doing what I'm proposing, and treating EINTR as success:

There's a handful of places where we're treating it as a failure, but I'm not sure if they actually matter as I'm not familiar with how the return value is used:

Imo, we should adjust one set of the above to be the same as the other - whichever one is preferred - ignoring EINTR should be correct to do on Apple platforms with the new define & Linux - what other platforms do we support where it may be a concern?

And here's actually another spot where we're retrying that I'll fix up along with the sharedmemory.cpp one:

@jkotas
Copy link
Member

jkotas commented Jul 6, 2025

There's a handful of places where we're treating it as a failure, but I'm not sure if they actually matter as I'm not familiar with how the return value is used:

As far as I can tell, the return value always ends up being ignored for the managed ones in your list. I guess we can ignore EINTR in SystemNative_Close to make it easier to follow.

runtime/src/coreclr/pal/src/misc/perfjitdump.cpp should be fixed to ignore the return value from close.

what other platforms do we support where it may be a concern?

I believe HP-UX is the only platform where it is correct to retry when close return EINTR. We do not support HP-UX. So it can be ignored unconditionally.

@teo-tsirpanis teo-tsirpanis added area-PAL-coreclr and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 6, 2025
Fix 1 remaining case of `close` being called in a loop
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

Thanks

@jkotas jkotas merged commit 8dfc3ad into dotnet:main Jul 13, 2025
156 of 161 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Aug 13, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-PAL-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider using $NOCANCEL versions of open and close on OSX-like platforms
3 participants