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

feat: support events is zero for uv_poll #702

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

uudiin
Copy link
Contributor

@uudiin uudiin commented May 15, 2024

As the poll() manual says that events field may be specified as zero, in which case the only events that can be returned in revents are POLLHUP, POLLERR, and POLLNVAL. This patch allows luv to support this call method with events equal zero. When the parameters in the lua is an empty string, it means that events is zero.

As the poll() manual says that events field may be specified as
zero, in which case the only events that can be returned in revents
are POLLHUP, POLLERR, and POLLNVAL.  This patch allows luv to support
this call method with events equal zero. When the parameters in the
lua is an empty string, it means that events is zero.

Signed-off-by: Tianjia Zhang <[email protected]>
@zhaozg
Copy link
Member

zhaozg commented Nov 5, 2024

LGTM, please @squeek502 give suggestion

@squeek502
Copy link
Member

squeek502 commented Nov 6, 2024

I'm not sure the changes in this PR would have the intended effect judging by the Libuv implementation of uv_poll_start. In both win and unix implementations, events == 0 is an early return:

https://github.com/libuv/libuv/blob/d4ab6fbba4669935a6bc23645372dfe4ac29ab39/src/win/poll.c#L498-L501

https://github.com/libuv/libuv/blob/d4ab6fbba4669935a6bc23645372dfe4ac29ab39/src/unix/poll.c#L137-L138


Other minor things:

  • Not sure why the CI was failing, will likely want to rebase the PR
  • This API currently doesn't have any tests in Luv, would be nice to add at least one (not necessarily a blocker for this PR, but would at least allow us to test that these changes work as intended)

@zhaozg
Copy link
Member

zhaozg commented Nov 16, 2024

Please rebase, wait CI pass to merge the PR.

Copy link
Member

@squeek502 squeek502 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 think this should be merged until it's proven that passing 0 for events has the intended effect.

@zhaozg
Copy link
Member

zhaozg commented Nov 17, 2024

I don't think this should be merged until it's proven that passing 0 for events has the intended effect.

sure

@truemedian
Copy link
Member

Following the libuv definitions:

For windows, passing events=0 is the canonical behavior of uv_poll_stop.
For unix, passing events=0 is the canonical behavior of uv_poll_stop, with an additional check that the watcher already exists.

I'm definitely leaning towards this being not desirable behavior, if you want events=0 you should call poll_stop.

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.

4 participants