-
Notifications
You must be signed in to change notification settings - Fork 798
listen:detect backlog parameter as in std. library #1896
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
listen:detect backlog parameter as in std. library #1896
Conversation
Define the `listen` backlog parameters as in the standard library. On one hand, this helps avoid hardcoded or unsynchronized values. On the other hand, it allows better control of default values depending on the target. This is an initial version which deals with issue tokio-rs#1872
src/sys/mod.rs
Outdated
#[allow(dead_code)] | ||
#[cfg(any( | ||
// Silently capped to `/proc/sys/net/core/somaxconn`. | ||
target_os = "linux", | ||
// Silently capped to `kern.ipc.soacceptqueue`. | ||
target_os = "freebsd", | ||
// Silently capped to `kern.somaxconn sysctl`. | ||
target_os = "openbsd", | ||
// Silently capped to the default 128. | ||
target_vendor = "apple", | ||
))] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting is broken here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I’ll double-check. I already ran cargo fmt
before submitting the PR
src/sys/mod.rs
Outdated
/// include: -1, 128, 1024, and 4096. | ||
/// | ||
#[allow(dead_code)] | ||
pub(crate) const fn get_listen_backlog_size() -> i32 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this be a constant (not function)?
Also it shouldn't need allow(dead_code)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thx.
- Yes, it's possible to have constant. I started with this approach, but I felt that the function better combines logically related code into one piece. Anyway, I’ll prepare a version using a constant and send it for review.
- Removing
allow(dead_code)
causes fails ifnet
feature is not enabled.
cargo build
Compiling log v0.4.22
Compiling mio v1.0.4 (/mnt/public-storage/projects/mio)
error: function `get_listen_backlog_size` is never used
--> src/sys/mod.rs:101:21
|
101 | pub(crate) const fn get_listen_backlog_size() -> i32 {
| ^^^^^^^^^^^^^^^^^^^^^^^
|
note: the lint level is defined here
--> src/lib.rs:6:5
|
6 | dead_code
| ^^^^^^^^^
error: could not compile `mio` (lib) due to 1 previous error
target_os = "espidf", | ||
target_os = "horizon" | ||
))] | ||
pub(crate) const LISTEN_BACKLOG_SIZE: i32 = 128; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cfg_select!
once stable would be handy here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I agree. It’s pretty wordy now
CI found one actual error:
I think the rest can be ignored, a rebase should fix them. |
- backlog size is now a constant, not a function - removed unused import (windows) - improved formatting
a789532
to
b0a951b
Compare
The 3 remaining failures seem legit, |
Good to know. Should I also add some fallback or special values for those targets? Following the STD is good, but not if it’s breaking things that were previously buildable |
Ideally we can add |
63b12cd
to
11380e8
Compare
Thanks @alexs-sh |
FWIW code you referenced is for unix socket. For TCP socket default is 128. The change seems good anyway. |
Define the default
listen
backlog close to the standard library.This helps avoid hardcoded unsynchronized values and allows better control of default values depending on the target. This is the initial version of the change that addresses issue #1872
Selecting a “valid” default value can be tricky due to:
As the topic could affect many targets, reviews, comments, and reports are very welcome.
Thank you