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

Add Socket::new_nonblocking #293

Closed
wants to merge 1 commit into from

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Feb 6, 2022

Equivalent to:

#[cfg(any(
    target_os = "android",
    target_os = "dragonfly",
    target_os = "freebsd",
    target_os = "fuchsia",
    target_os = "illumos",
    target_os = "linux",
    target_os = "netbsd",
    target_os = "openbsd"
))]
let ty = ty.nonblocking();

let socket = Socket::new(domain, ty, protocol)?;

#[cfg(not(any(
    target_os = "android",
    target_os = "dragonfly",
    target_os = "freebsd",
    target_os = "fuchsia",
    target_os = "illumos",
    target_os = "linux",
    target_os = "netbsd",
    target_os = "openbsd"
)))]
socket.set_nonblocking(true)?;

I've seen code like this several times and thought it would be useful to have a constructor that does the same thing.

Copy link
Collaborator

@chansuke chansuke left a comment

Choose a reason for hiding this comment

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

I appreciate your work. Seems good for me.

@Thomasdezeeuw
Copy link
Collaborator

I'm not sure about this. I understand you wanting to reduce the code duplication, but where do we draw the line when it comes to constructors? We already have two variants: "new" and "raw", do we also want raw_nonblocking? What about pair_nonblocking? etc.

@Darksonn
Copy link
Collaborator

Darksonn commented Feb 6, 2022

Maybe there's a more general way to do this where we define a flags type which has a nonblocking flag, then use that?

@Thomasdezeeuw
Copy link
Collaborator

Maybe there's a more general way to do this where we define a flags type which has a nonblocking flag, then use that?

We could add our own bit to Type on the OSs that don't support Type::non_blocking (making it available on all OSs) and make the Socket::set_nonblocking call inside Socket::new. Only difficulty here is ensuring the bit is never used by an OS flag (like SOCK_NONBLOCK).

@hawkw
Copy link

hawkw commented Feb 6, 2022

I'm not sure about this. I understand you wanting to reduce the code duplication, but where do we draw the line when it comes to constructors? We already have two variants: "new" and "raw", do we also want raw_nonblocking? What about pair_nonblocking? etc.

This seems like a potential argument for a SocketBuilder or something?

let sock = SocketBuilder::new()
    .nonblocking(true)
    .finish(domain, ty, protocol);

let raw = SocketBuilder::new()
    .nonblocking(true)
    .finish_raw(domain, ty, protocol);

or similar?

@Thomasdezeeuw
Copy link
Collaborator

This seems like a potential argument for a SocketBuilder or something?

let sock = SocketBuilder::new()
    .nonblocking(true)
    .finish(domain, ty, protocol);

let raw = SocketBuilder::new()
    .nonblocking(true)
    .finish_raw(domain, ty, protocol);

or similar?

We already have that in the Domain, Type Protocol types. It just that currently socket2 is mostly just wraps around system calls where 1 function = 1 system call, thus if the OS doesn't support a flag, like SOCK_NONBLOCK here, socket2 currently leaves it up to the user to handle properly. We could break from 1 function = 1 system call though and implement these kind of niceties in Socket::new.

@taiki-e
Copy link
Member Author

taiki-e commented Jun 17, 2022

Closing in favor of the idea proposed in #293 (comment).

@taiki-e taiki-e closed this Jun 17, 2022
@taiki-e taiki-e deleted the socket-new-nonblocking branch June 17, 2022 05:43
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.

5 participants