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

[libs] Fix poll events conversion #86843

Merged
merged 1 commit into from
Jun 30, 2023

Conversation

trungnt2910
Copy link
Contributor

  • Use bitwise and to check for all flags instead of using a switch statement for poll events flags conversion.
  • Convert poll events before comparison in assert statement.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label May 28, 2023
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label May 28, 2023
@stephentoub
Copy link
Member

Why?

@trungnt2910
Copy link
Contributor Author

The problem does not appear on Linux or other UNIXes as they use the same values as the PollEvents enum in the PAL.

However, there don't seem to be any standard mandating the underlying values of these flags, and on Haiku these flags have different values.

Because there are no standards for the values, I don't think it is correct to assume the values are the same for the PAL and the underlying platform.

As this change affects cross-platform code I decided to open a PR separate from my Haiku-specific changes.

@stephentoub
Copy link
Member

I'm not understanding. The whole purpose of these methods is to map between the PAL values and the OS values which may not match. You're saying your new version works and the existing ones fail?

@trungnt2910
Copy link
Contributor Author

trungnt2910 commented May 28, 2023

Unless there is something I am missing, the existing code has two issues:

  • It uses switch to map a set of flags between PAL and OS values:

switch (event->Events)
{
case PAL_POLLIN:
pollfds[i].events = POLLIN;
break;
#ifdef POLLPRI // not available in WASI
case PAL_POLLPRI:
pollfds[i].events = POLLPRI;
break;
#endif
case PAL_POLLOUT:
pollfds[i].events = POLLOUT;
break;
case PAL_POLLERR:
pollfds[i].events = POLLERR;
break;
case PAL_POLLHUP:
pollfds[i].events = POLLHUP;
break;
case PAL_POLLNVAL:
pollfds[i].events = POLLNVAL;
break;
default:
pollfds[i].events = event->Events;
break;
}

When a set containing two or more flags is passed (for example, PAL_POLLIN | PAL_POLLOUT), the existing code falls to the default label which does no mapping at all.

The same applies to the reverse mapping:

switch (pfd->revents)
{
case POLLIN:
pollEvents[i].TriggeredEvents = PAL_POLLIN;
break;
#ifdef POLLPRI // not available in WASI
case POLLPRI:
pollEvents[i].TriggeredEvents = PAL_POLLPRI;
break;
#endif
case POLLOUT:
pollEvents[i].TriggeredEvents = PAL_POLLOUT;
break;
case POLLERR:
pollEvents[i].TriggeredEvents = PAL_POLLERR;
break;
case POLLHUP:
pollEvents[i].TriggeredEvents = PAL_POLLHUP;
break;
case POLLNVAL:
pollEvents[i].TriggeredEvents = PAL_POLLNVAL;
break;
default:
pollEvents[i].TriggeredEvents = (int16_t)pfd->revents;
break;
}

  • It uses a runtime assert to check that the integer representation of the converted flags is equal to that of the PAL flags:

assert(pfd->events == pollEvents[i].Events);

The assert seems to have meant to ensure that the poll syscall did not modify fields it is not supposed to touch.

Neither of these issues are visible on Linux and some other UNIXes since the PAL flags and the OS flags are the same (probably the flags were chosen to match Linux and other platforms all coincidentally have the same flags?). The default block on these platforms still produce seemingly correct results and the assert still passes since no conversion is actually done.

On Haiku, or any other platforms with a different representation of the poll flags, the problems become apparent. For these platforms, it is true that my version works and the existing ones fail.

@danmoseley danmoseley 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 Jun 1, 2023
@ghost
Copy link

ghost commented Jun 7, 2023

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

Issue Details
  • Use bitwise and to check for all flags instead of using a switch statement for poll events flags conversion.
  • Convert poll events before comparison in assert statement.
Author: trungnt2910
Assignees: -
Labels:

area-System.IO, area-PAL-coreclr, community-contribution

Milestone: -

@ghost
Copy link

ghost commented Jun 7, 2023

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

Issue Details
  • Use bitwise and to check for all flags instead of using a switch statement for poll events flags conversion.
  • Convert poll events before comparison in assert statement.
Author: trungnt2910
Assignees: -
Labels:

area-System.IO, area-System.Net.Sockets, community-contribution

Milestone: -

- Use bitwise and to check for all flags instead of using a switch
statement for poll events flags conversion.
- Convert poll events before comparison in assert statement.
Copy link
Member

@wfurt wfurt left a comment

Choose a reason for hiding this comment

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

LGTM

@stephentoub stephentoub merged commit e7e9925 into dotnet:main Jun 30, 2023
@karelz karelz added this to the 8.0.0 milestone Jul 3, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Aug 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Net.Sockets 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.

7 participants