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

Update mio to 0.8 #4270

Merged
merged 7 commits into from
Dec 31, 2021
Merged

Update mio to 0.8 #4270

merged 7 commits into from
Dec 31, 2021

Conversation

taiki-e
Copy link
Member

@taiki-e taiki-e commented Nov 23, 2021 •
edited
Loading

Closes #4135

Unblocks #3082, #3312, #3542, #3545, #3170, #3309

@taiki-e taiki-e added A-tokio Area: The main tokio crate M-net Module: tokio/net labels Nov 23, 2021
@taiki-e taiki-e marked this pull request as ready for review November 23, 2021 04:48
@taiki-e taiki-e requested a review from Darksonn November 23, 2021 05:16
@taiki-e
Copy link
Member Author

taiki-e commented Nov 23, 2021

As for the socket2 issue, I don't think it should be considered a blocker, but if one of the reviewers wants to make it a blocker, I'm fine with that.

Darksonn
Darksonn reviewed Nov 23, 2021
View reviewed changes
tokio/src/net/tcp/socket.rs
Comment on lines -315 to +323
self.inner.get_send_buffer_size()
self.inner.send_buffer_size().map(|n| n as u32)
Copy link
Contributor

Choose a reason for hiding this comment

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

There are several casts in this PR. Do we risk them truncating any data here?

Copy link
Member Author

Choose a reason for hiding this comment

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

There are 5 casts:

  • set_send_buffer_size,set_recv_buffer_size (cast u32 as usize): on tokio, usize is always at least 32 bits.
  • send_buffer_size,recv_buffer_size (cast usize as u32): socket2 gets these values as c_int (always i32) and then cast them to usize (1, 2).
  • listen (cast u32 as i32): It seems using cast is wrong, I'll fix this.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in 5326df1

tokio/Cargo.toml
"mio/tcp",
"mio/udp",
"mio/uds",
"socket2/all",
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not too familiar with the features of socket2, but can we be more precise here?

Copy link
Member Author

Choose a reason for hiding this comment

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

socket2's all feature means enabling APIs that are not available in some OSes, not means enabling all feature flags.

https://github.com/rust-lang/socket2/blob/6601ed132b37d6e9d178b34918bfb0b236800232/Cargo.toml#L42-L43

# Enable all API, even ones not available on all OSs.
all = []

In our case, this feature is needed to use reuse_port/set_reuse_port, which is not available in Solaris & Illumos.

@taiki-e taiki-e requested a review from Darksonn November 23, 2021 10:43
Darksonn
Darksonn approved these changes Nov 23, 2021
View reviewed changes
Copy link
Contributor

@Darksonn Darksonn left a comment

Choose a reason for hiding this comment

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

The change itself seems good, though I'd like to discuss it this evening when the americans come online.

Noah-Kennedy
Noah-Kennedy approved these changes Nov 25, 2021
View reviewed changes
Copy link
Contributor

@Noah-Kennedy Noah-Kennedy left a comment

Choose a reason for hiding this comment

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

This looks good.

zh-jq
zh-jq reviewed Nov 26, 2021
View reviewed changes
tokio/src/net/tcp/socket.rs
@@ -500,7 +524,7 @@ impl TcpSocket {
///
/// #[tokio::main]
/// async fn main() -> std::io::Result<()> {
///
///
/// let socket2_socket = Socket::new(Domain::IPV4, Type::STREAM, None)?;
///
/// let socket = TcpSocket::from_std_stream(socket2_socket.into());
Copy link
Contributor

Choose a reason for hiding this comment

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

How about impl From<socket2::Socket> for TcpSocket?

Copy link
Member Author

Choose a reason for hiding this comment

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

We don't want to make a crate with version 0.x a public dependency of a crate with version 1.x.

See also api-guidelines: https://rust-lang.github.io/api-guidelines/necessities.html#public-dependencies-of-a-stable-crate-are-stable-c-stable

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, once this PR is merged, adding the options that socket2 has to the TcpSocket will be easy. So the cases where code like this example is needed should be greatly reduced in the near future.

@taiki-e taiki-e requested a review from carllerche November 28, 2021 14:36
@taiki-e
Copy link
Member Author

taiki-e commented Dec 31, 2021

I got two approvals and no one had raised any objections for over a month, so I'll merge this.

@taiki-e taiki-e merged commit ee0e811 into master Dec 31, 2021
@taiki-e taiki-e deleted the taiki-e/mio branch December 31, 2021 03:28
This was referenced Dec 31, 2021
Merged
Merged
taiki-e added a commit that referenced this pull request Jan 11, 2022
@taiki-e
net: fix TcpSocket bugs introduced in #4270
f011db9
taiki-e added a commit that referenced this pull request Jan 11, 2022
@taiki-e
net: fix TcpSocket bugs introduced in #4270
ee25597
taiki-e added a commit that referenced this pull request Jan 11, 2022
@taiki-e
net: fix TcpSocket bugs introduced in #4270
d970e3a
@taiki-e taiki-e mentioned this pull request Jan 11, 2022
Closed
taiki-e added a commit that referenced this pull request Jan 11, 2022
@taiki-e
Revert "Update mio to 0.8 (#4270)"
6ab451e 
This reverts commit ee0e811.
carllerche added a commit that referenced this pull request Jan 11, 2022
@carllerche
Revert "Update mio to 0.8 (#4270)" and dependent changes
99a10b8 
This reverts commits:
 * ee0e811
 * 49a9dc6
 * 0190831
 * 43cdb2c
 * 96370ba
 * a9d9bde
carllerche added a commit that referenced this pull request Jan 11, 2022
@carllerche
Revert "Update mio to 0.8 (#4270)" and dependent changes (#4392)
aea26b3 
This reverts commits:
 * ee0e811
 * 49a9dc6
 * 0190831
 * 43cdb2c
 * 96370ba
 * a9d9bde
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zh-jq zh-jq zh-jq left review comments

@Darksonn Darksonn Darksonn approved these changes

@Noah-Kennedy Noah-Kennedy Noah-Kennedy approved these changes

@carllerche carllerche Awaiting requested review from carllerche

Assignees
No one assigned
Labels
A-tokio Area: The main tokio crate M-net Module: tokio/net
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Updating to Mio v0.8
4 participants
@taiki-e @Darksonn @Noah-Kennedy @zh-jq