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

DRAFT: trying to make add safe #192

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

Conversation

keepsimple1
Copy link

To follow up on the issue #191, I'm experimenting with some changes to make Poller::add safe.

Since I'm new to I/O Safety, I probably missed something or maybe totally wrong. But I wanted to post this PR to get comments and find out what I've missed at least.

This draft change only applied on macOS, i.e. kqueue as I'm trying to see if I was on the right direction. Thanks in advance.

@keepsimple1 keepsimple1 marked this pull request as draft February 15, 2024 04:57
let rawfd = fd.as_raw_fd();

// SAFETY: `rawfd` is valid as it is from `BorrowedFd`. And
// this block never closes / deletes `rawfd`.
Copy link
Author

Choose a reason for hiding this comment

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

Does this comment make sense at all? I'm new to I/O safety, please advise.

@notgull
Copy link
Member

notgull commented Apr 22, 2024

I don't think it's possible to make add safe in the general case; the Windows and poll()-based backends can't be made safe. However it is possible to make it safe for certain backends.

@keepsimple1
Copy link
Author

I don't think it's possible to make add safe in the general case; the Windows

What if we change RawSocket to BorrowedSocket in the following? Will that help?

socket: RawSocket,

and poll()-based backends can't be made safe. However it is possible to make it safe for certain backends.

Is it this one? (seems it is still / already safe?)

pub fn add(&self, fd: RawFd, ev: Event, mode: PollMode) -> io::Result<()> {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants