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

net: add TcpSocket::set_{send, recv}_buffer_size #1384

Merged
merged 9 commits into from
Nov 2, 2020
Merged

Conversation

hawkw
Copy link
Member

@hawkw hawkw commented Oct 31, 2020

This commit adds methods for setting SO_RECVBUF and SO_SNDBUF to
Mio's TcpSocket type. It would be nice to eventually expose these in
Tokio, so adding them to Mio is the first step.

See tokio-rs/tokio#3082 for details.

Signed-off-by: Eliza Weisman eliza@buoyant.io

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@Thomasdezeeuw
Copy link
Collaborator

Can you also add the getters and a simple round-trip test?

@hawkw
Copy link
Member Author

hawkw commented Oct 31, 2020

Can you also add the getters and a simple round-trip test?

Sure, will do!

hawkw added 3 commits October 31, 2020 12:57
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw hawkw force-pushed the eliza/more-sockopts branch from 8eea949 to d7da7cd Compare October 31, 2020 20:30
tests/tcp_socket.rs Outdated Show resolved Hide resolved
src/net/tcp/socket.rs Show resolved Hide resolved
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
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.

LGTM, see my comment in the test to fix the CI.

src/net/tcp/socket.rs Show resolved Hide resolved
tests/tcp_socket.rs Outdated Show resolved Hide resolved
hawkw added 3 commits November 1, 2020 10:36
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
apparently two casts are required for this because it's a `*mut i8`

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
Signed-off-by: Eliza Weisman <eliza@buoyant.io>
@hawkw
Copy link
Member Author

hawkw commented Nov 2, 2020

It looks like one of the test is failing on FreeBSD CI:

---- no_events_after_deregister stdout ----
thread 'no_events_after_deregister' panicked at 'called `Result::unwrap()` on an `Err` value: Os { code: 57, kind: NotConnected, message: "Socket is not connected" }', tests/tcp_stream.rs:492:35
stack backtrace:
   0:  0x110dfb3 - <unknown>
   1:  0x113043e - <unknown>
   2:  0x109c89d - <unknown>
   3:  0x11066b8 - <unknown>
   4:  0x1110020 - <unknown>
   5:  0x110fcb5 - <unknown>
   6:  0x11106c0 - <unknown>
   7:  0x1110271 - <unknown>
   8:  0x110e3fa - <unknown>
   9:  0x1110221 - <unknown>
  10:  0x112ecda - <unknown>
  11:  0x112eb0a - <unknown>
  12:  0x105a0c4 - <unknown>
  13:  0x1055210 - <unknown>
  14:  0x1069573 - <unknown>
  15:  0x106f3f4 - <unknown>
  16:  0x10c1cd3 - <unknown>
  17:  0x10c02ab - <unknown>
  18:  0x109bc43 - <unknown>
  19:  0x10a10b9 - <unknown>
  20:  0x1117090 - <unknown>
  21: 0x211848ab - <unknown>


failures:
    no_events_after_deregister

It seems quite unlikely that the changes in this PR could have actually broken this, since the test that's failing doesn't do anything related to send/recv buffer size:

mio/tests/tcp_stream.rs

Lines 473 to 506 in 7358a31

#[test]
fn no_events_after_deregister() {
let (mut poll, mut events) = init_with_poll();
let (thread_handle, address) = echo_listener(any_local_address(), 1);
let mut stream = TcpStream::connect(address).unwrap();
poll.registry()
.register(&mut stream, ID1, Interest::WRITABLE.add(Interest::READABLE))
.expect("unable to register TCP stream");
poll.registry()
.deregister(&mut stream)
.expect("unable to deregister TCP stream");
expect_no_events(&mut poll, &mut events);
// We do expect to be connected.
assert_eq!(stream.peer_addr().unwrap(), address);
// Also, write should work
let mut buf = [0; 16];
assert_would_block(stream.peek(&mut buf));
assert_would_block(stream.read(&mut buf));
checked_write!(stream.write(&DATA1));
stream.flush().unwrap();
expect_no_events(&mut poll, &mut events);
drop(stream);
thread_handle.join().expect("unable to join thread");
}

I'm guessing this is flaky, so I went ahead and restarted it. @Thomasdezeeuw have you seen this test failing spuriously before?

@hawkw hawkw merged commit 40c4af7 into master Nov 2, 2020
@Thomasdezeeuw Thomasdezeeuw deleted the eliza/more-sockopts branch November 3, 2020 08:21
hawkw added a commit to tokio-rs/tokio that referenced this pull request Nov 16, 2020
This commit adds `set_{send, recv}_buffer_size` methods to `TcpSocket`
for setting the size of the TCP send and receive buffers, and `{send,
recv}_buffer_size` methods for returning the current value. These just
call into similar methods on `mio`'s `TcpSocket` type, which were added
in tokio-rs/mio#1384.

Refs: #3082

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit to tokio-rs/tokio that referenced this pull request Nov 16, 2020
This commit adds `set_{send, recv}_buffer_size` methods to `TcpSocket`
for setting the size of the TCP send and receive buffers, and `{send,
recv}_buffer_size` methods for returning the current value. These just
call into similar methods on `mio`'s `TcpSocket` type, which were added
in tokio-rs/mio#1384.

Refs: #3082

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
hawkw added a commit to tokio-rs/tokio that referenced this pull request Nov 16, 2020
This commit adds `set_{send, recv}_buffer_size` methods to `TcpSocket`
for setting the size of the TCP send and receive buffers, and `{send,
recv}_buffer_size` methods for returning the current value. These just
call into similar methods on `mio`'s `TcpSocket` type, which were added
in tokio-rs/mio#1384.

Refs: #3082

Signed-off-by: Eliza Weisman <eliza@buoyant.io>
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