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

fix broken Select with error list on macOS #104915

Merged
merged 12 commits into from
Jul 28, 2024
Merged

fix broken Select with error list on macOS #104915

merged 12 commits into from
Jul 28, 2024

Conversation

wfurt
Copy link
Member

@wfurt wfurt commented Jul 15, 2024

as I discover on #920, we can make the Select work if we use

 struct pollfd polls[1];

        polls[0].fd = sockfd;
        polls[0].events = POLLIN | POLLHUP | POLLPRI;

        int err = poll(&polls, 1, 1000);

instead of

 struct pollfd polls[2];

        polls[0].fd = sockfd;
        polls[0].events = POLLIN | POLLHUP ;
        polls[1].fd = sockfd;
        polls[1].events = POLLPRI;

        int err = poll(&polls, 2, 1000);

e.g on OSX like platforms we walk the lists to avoid socks duplication between read and error list.
It does add some expense but the Select should not be used for massive amount of sockets anyway and the fix makes the simple cases consistent with other platforms. Right now it just hangs and that is unpleasant.

fixes #920

@wfurt wfurt requested a review from a team July 15, 2024 19:09
@wfurt wfurt self-assigned this Jul 15, 2024
Copy link
Contributor

Tagging subscribers to this area: @dotnet/ncl
See info in area-owners.md if you want to be subscribed.

@@ -19,6 +19,8 @@ internal static partial class SocketPal
public static readonly int MaximumAddressSize = Interop.Sys.GetMaximumAddressSize();
private static readonly bool SupportsDualModeIPv4PacketInfo = GetPlatformSupportsDualModeIPv4PacketInfo();

private static readonly bool PollNeedsErrorListFixup = OperatingSystem.IsMacOS() || OperatingSystem.IsIOS() || OperatingSystem.IsTvOS();
Copy link
Member

Choose a reason for hiding this comment

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

I assume there's nothing we can query for in the PAL layer and it really is just us knowing that these OSes behave differently?

Copy link
Member Author

Choose a reason for hiding this comment

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

If there is update we can check Darwin kernel version. But I'm not aware of anything so far.

// To fix that we will search read list and if macthing descriptor exiost we will add events flags
// instead of adding new entry to error list.
int readIndex = 0;
while (readIndex < readCount)
Copy link
Member

Choose a reason for hiding this comment

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

Is this turning a linear operation into an N^2 operation?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes. This is where the assumption comes in that this is used from small number of sockets where it does not matter as much .... and the cost is only on platforms that are currently broken. (and use read and error list together)

Copy link
Member

Choose a reason for hiding this comment

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

@GrabYourPitchforks, any concerns?

Copy link
Member Author

Choose a reason for hiding this comment

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

I put up different implementation that avoids the N^2 problem. Please take another look @stephentoub

src/native/libs/System.Native/pal_networking.c Outdated Show resolved Hide resolved
src/native/libs/System.Native/pal_networking.c Outdated Show resolved Hide resolved
for (int i = 0 ; i < readFdsCount; i++)
{
fd = *(readFds + i);
*(readFds + i) = __DARWIN_FD_ISSET(fd, readSetPtr);
Copy link
Member

Choose a reason for hiding this comment

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

What does __DARWIN_FD_ISSET return? Is it returning fd if it's set and otherwise 0?

Copy link
Member Author

Choose a reason for hiding this comment

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

this is basically same as FD_ISSET but working on set larger than FD_SETSIZE. man page says

FD_ISSET(fd, &fdset) is non-zero if fd is a member of fdset, zero otherwise.

in practice this seems to be int with one bit set depending on the position in it bit array.

Comment on lines 1818 to 1820
Span<int> readFDs = checkRead?.Count > 20 ? new int[checkRead.Count] : stackalloc int[checkRead?.Count ?? 0];
Span<int> writeFDs = checkWrite?.Count > 20 ? new int[checkWrite.Count] : stackalloc int[checkWrite?.Count ?? 0];
Span<int> errorFDs = checkError?.Count > 20 ? new int[checkError.Count] : stackalloc int[checkError?.Count ?? 0];
Copy link
Member

Choose a reason for hiding this comment

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

Where does the "20" come from? Pull it out into a named constant?

wfurt and others added 2 commits July 22, 2024 11:50
Co-authored-by: Stephen Toub <stoub@microsoft.com>
@wfurt wfurt merged commit cb5acf5 into dotnet:main Jul 28, 2024
109 checks passed
@wfurt wfurt deleted the macSelect branch July 28, 2024 16:46
@github-actions github-actions bot locked and limited conversation to collaborators Aug 30, 2024
@karelz karelz added this to the 9.0.0 milestone Sep 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Socket.Select() method doesn't work as expected on macOS
3 participants