Fix Windows build of ds-ipc-pal-socket.c and remove EP_ASSERT around fcntl FD_CLOEXEC calls. #56141
Conversation
|
/CC @josalem |
…ort. Up until dotnet#55850 it was possible to build using diagnostic server socket PAL on Windows for local testing. NOTE, this is not a config we build on CI since it is not used by any products (diagnostic server PAL on Windows uses NamedPipes), but useful for local testing. The same PR also included calls to fcntl using FD_CLOEXEC in case platform doesn't define SOCK_CLOEXEC. Since this is a Linux specific flag, it might not be defined on several targeted platforms. Failures calling fcntl using FD_CLOEXEC was also triggering an EP_ASSERT but that is only on debug builds. Most calls to fcntl using FD_CLOEXEC in runtime PAL layers (both CoreCLR and MonoVM) doesn't check for errors in this case and sees the operation as best effort, like done here https://github.com/dotnet/runtime/blob/b937677e8f8601848d29bc072a93cc0c6e21576d/src/libraries/Native/Unix/System.Native/pal_networking.c#L2499. We should do the same in ds-ipc-pal-socket.c making sure it won't break any potential platform not fully supporting it or if we are really concerned about this error, handle it as real error failing creation of socket, but that needs to be tested on mobile platforms (not running TCP/IP PAL on CI).
lateralusX
force-pushed
the
lateralusX/adjust-ds-pal-fcntl-calls
branch
from
eb1e93b to
7470851
Compare
| if (fcntl (new_socket, F_SETFD, FD_CLOEXEC) == -1) { | ||
| EP_ASSERT (!"Failed to set CLOEXEC"); | ||
| } | ||
| #ifndef HOST_WIN32 |
There was a problem hiding this comment.
This should still check the existence of SOCK_CLOEXEC so we only do the fcntl call if we didn't already set it during the call to socket.
There was a problem hiding this comment.
It is already under #ifndef SOCK_CLOEXEC, or maybe you though about adding check for FD_CLOEXEC as well? That is not something we have done in other places doing similar code (just checking SOCK_CLOEXEC), and I know that some Mono headers actually define this on platforms where its not used/available, so on those platforms the additional check won't make any difference.
There was a problem hiding this comment.
Ah, no I missed line 355 because I was reviewing this on my phone 🙄. If we're checking ifndef SOCK_CLOEXEC this is fine.
nit: #if !defined(SOCK_CLOEXEC) && !defined(HOST_WIN32)?
| if (fcntl (new_socket, F_SETFD, FD_CLOEXEC) == -1) { | ||
| EP_ASSERT (!"Failed to set CLOEXEC"); | ||
| } | ||
| #ifndef HOST_WIN32 |
There was a problem hiding this comment.
Ah, no I missed line 355 because I was reviewing this on my phone 🙄. If we're checking ifndef SOCK_CLOEXEC this is fine.
nit: #if !defined(SOCK_CLOEXEC) && !defined(HOST_WIN32)?
ghost
locked as resolved and limited conversation to collaborators
2 participants
Up until #55850 it was possible to build using diagnostic server socket PAL on Windows for local testing. NOTE, this is not a config we build on CI since it is not used by any products (diagnostic server PAL on Windows uses NamedPipes), but useful for local testing.
The same PR also included calls to fcntl using FD_CLOEXEC in case platform doesn't define SOCK_CLOEXEC. Since this is a Linux specific flag, it might not be defined on several targeted platforms. Failures calling fcntl using FD_CLOEXEC was also triggering an EP_ASSERT but that is only on debug builds. Most calls to fcntl using FD_CLOEXEC in runtime PAL layers (both CoreCLR and MonoVM) doesn't check for errors in this case and sees the operation as best effort, like done here:
runtime/src/libraries/Native/Unix/System.Native/pal_networking.c
Line 2499 in b937677
We should do the same in ds-ipc-pal-socket.c making sure it won't break any potential platform not fully supporting it or if we are really concerned about this error, handle it as real error failing creation of socket, but that needs to be tested on mobile platforms (we are not running TCP/IP PAL for mobile platforms on CI), so just handle it as done elsewhere in PAL (on best effort), should be enough.