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 Event.write to have writable = true #15

Merged
merged 1 commit into from
Jul 2, 2023

Conversation

edwintorok
Copy link
Contributor

I was testing the library and my code got "stuck", I tried using Poll.set mux socket Poll.Event.write to handle EINPROGRESS from connect, but strace was showing that the FD was not getting added to the epoll set.
This was because the code thought no change was requested, since Event.write was equal to Event.none.

Copy link
Owner

@anuragsoni anuragsoni left a comment

Choose a reason for hiding this comment

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

This was an embarrassing miss on my part!!

Thank you!

@anuragsoni anuragsoni merged commit e477cb2 into anuragsoni:main Jul 2, 2023
@edwintorok
Copy link
Contributor Author

Thanks for merging, there are still some other problems, e.g. 'oneshot' seems to be added to each event, so now after one event everything is still stuck.... (unless I keep changing epoll set)

@anuragsoni
Copy link
Owner

I’ve been thinking about allowing users to specify one shot vs level triggered events. Both kqueue and epoll support it so this seems like an option worth surfacing. I liked the idea of one shot events so a readiness event is only reported once instead of multiple times, and then considering file descriptors always ready unless encountering EAGAIN

@edwintorok
Copy link
Contributor Author

Giving control the user sounds right, I have no experience with kqueue though, so I"ll leave that improvement idea to you.

anuragsoni added a commit to anuragsoni/opam-repository that referenced this pull request Jul 2, 2023
CHANGES:

* Fix bug where writable events weren't setting writable flag to true (anuragsoni/poll#15, @edwintorok)
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.

2 participants