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.

Sign up for GitHub

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

Jump to bottom

net: add TcpSocket::{set, get}_keepalive #1385

Merged
merged 37 commits into from
Nov 11, 2020
Merged

net: add TcpSocket::{set, get}_keepalive #1385

merged 37 commits into from
Nov 11, 2020

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Oct 31, 2020 •
edited
Loading

This commit adds methods to TcpSocket to enable/disable TCP keepalive,
and set the keepalive idle time, probe interval, and retries, when the
OS supports configuring those values.

Because of how TCP keepalive is configured in Winsock, it's necessary to
use a builder-type API: the interval and idle time are set together as
separate fields on a struct, and if either is not set, it clobbers the
previous value. This means we can't just call that API twice, once with
each value set. Unlike Unix OSes, Windows also doesn't provide any way
to access the current value of these parameters, so we can't just
populate the other fields on the struct with the previous values, either
(incidentally, this also means we can't provide getters on Windows).

Signed-off-by: Eliza Weisman [email protected]

@hawkw hawkw mentioned this pull request Oct 31, 2020
Open
Thomasdezeeuw
Thomasdezeeuw reviewed Nov 1, 2020
View reviewed changes
src/net/tcp/socket.rs Outdated Show resolved Hide resolved
src/sys/windows/tcp.rs Outdated Show resolved Hide resolved
@hawkw
net: add TcpSocket::{set, get}_keepalive
6c00878 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
make rustfmt happy
f5c0857 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
windows: use i32::max_value
93b45d7 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
fixup windows
b9a3a52 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
maybe these pointer casts will be good
04cd34f 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
add doc_cfg
eb8ed01 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
undo rustfmt
e769f9d 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
apparently Macs do have TCP_KEEPINTVL/TCP_KEEPCNT
7c14e89 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
fix shell rt cfgs
51164b7 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
fix missing target_os cfgs
600a662 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
????
21c4c7b 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
maybe we can at least see what's going on
dcbd0ec 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
maybe this has to be a valid ptr???
bbeac6f 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
asdf
f0a704b 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
more println
2f9a2a4 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
agh
d251450 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
hmm let's see what happens if we use getsockopt
9bfad08 
i asked a friend who works at microsoft and she said to try this. the
docs don't mention it anywhere, though, so who knows what will happen!

Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
also remove printlns
21ff829 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
oops
4c1b6a9
@hawkw
lol oh no it's in a different header entirely
a75330c 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
AGH NO IT WAS A *char ALL ALONG
096fd96 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
Copy link
Member Author

hawkw commented Nov 6, 2020

Okay, I've updated this branch to use a builder-style API to workaround the issues with winsock, which I've described in the PR description. Let me know what you think!

@hawkw
blurgh, shell
942a53d 
Signed-off-by: Eliza Weisman <[email protected]>
@hawkw
nicer cfgs
b83957d 
Signed-off-by: Eliza Weisman <[email protected]>
@Darksonn Darksonn mentioned this pull request Nov 8, 2020
Closed
Thomasdezeeuw
Thomasdezeeuw approved these changes Nov 9, 2020
View reviewed changes
Copy link
Collaborator

@Thomasdezeeuw Thomasdezeeuw left a comment

Choose a reason for hiding this comment

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

Two small things, but LGTM.

This might be the nicest, full-featured TCP keepalive API I've seen, well done.

src/net/tcp/socket.rs Outdated
/// will start sending keepalive messages on an idle connection.
///
/// Additionally, this will enable TCP keepalive on this socket, if it is
/// not already enabled.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you add a # Notes section saying that all parameters related to keepalive will be overwritten.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm, that's actually only the case on Windows; on Unix, we can avoid overwriting previously set parameters. Do you think we should have the same behavior on Unix OSes even though it's less than ideal?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's change it to that it might overwrite other keepalive parameters and that either only a single call should be made, or that all desired parameters should be set in all calls. I don't want to make unnecessary system calls on Unix.

src/sys/unix/tcp.rs Outdated Show resolved Hide resolved
@hawkw
Copy link
Member Author

hawkw commented Nov 9, 2020

This might be the nicest, full-featured TCP keepalive API I've seen, well done.

Thanks! :) I noticed that socket2's implementation set the same value for the keepalive timeout and keepalive interval on Windows, and thought "hmm, that doesn't seem ideal"...and then I went down a bit of a rabbit-hole when it turned out that doing something reasonable on winsock was a bit harder than it looked at first...

@pablosichert pablosichert mentioned this pull request Nov 11, 2020
Open
@Thomasdezeeuw Thomasdezeeuw merged commit 290c43a into master Nov 11, 2020
@Thomasdezeeuw Thomasdezeeuw deleted the eliza/even-more-sockopts branch November 11, 2020 10:13
@Thomasdezeeuw
Copy link
Collaborator

Thanks @hawkw.

@Thomasdezeeuw Thomasdezeeuw mentioned this pull request Nov 11, 2020
Closed
@hawkw hawkw mentioned this pull request Nov 16, 2020
Closed
@psarna psarna mentioned this pull request Dec 14, 2020
Closed
Thomasdezeeuw added a commit to Thomasdezeeuw/socket2 that referenced this pull request Dec 18, 2020
@Thomasdezeeuw
Adopt Mio's TCP keepalive API
97f39b1 
Originally implemented by @hawkw here in
tokio-rs/mio#1385.

Closed rust-lang#79.
Fixes rust-lang#24.
@Thomasdezeeuw Thomasdezeeuw mentioned this pull request Dec 18, 2020
Merged
Thomasdezeeuw added a commit to rust-lang/socket2 that referenced this pull request Dec 19, 2020
@Thomasdezeeuw
Adopt Mio's TCP keepalive API
c527a94 
Originally implemented by @hawkw here in
tokio-rs/mio#1385.

Closed #79.
Fixes #24.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Thomasdezeeuw Thomasdezeeuw Thomasdezeeuw approved these changes

@carllerche carllerche Awaiting requested review from carllerche

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@hawkw @Thomasdezeeuw @carllerche