Add checks in from_std to ensure that it is non-blocking

[kmod-midori]

Is your feature request related to a problem? Please describe.
Multiple users (including me) have fallen in to this trap. Getting a UdpSocket from some other libraries and forget to set it as non-blocking can hang the runtime in a way that is not easy to debug (especially for beginners).

Describe the solution you'd like
Some checks that ensures that the socket being passed in is indeed non-blocking. This is doable in Linux with fnctl, but I'm not sure about the status on Windows.

Such check can be designed so that it only preforms in debug mode, as extra syscalls are usually involved.

Ideally, this should trigger a panic if the check fails, but an error is better than nothing.

[stepancheg]

This is a big deal.

I spent several hours debugging the issue.

I can reproduce it with this minimal example:

use tokio::runtime::Runtime;
use tokio::net::TcpListener;
use tokio::io::AsyncReadExt;
use std::time::Duration;
use tokio::sync::oneshot;

fn main() {
    let reactor = Runtime::new().unwrap();
    let handle = reactor.handle().clone();

    reactor.block_on(async move {
        let listener = std::net::TcpListener::bind(("127.0.0.1:1234")).unwrap();

        let listener = TcpListener::from_std(listener).unwrap();

        println!("listening on port {}", listener.local_addr().unwrap().port());
        let h1 = handle.clone();
        handle.spawn(async move {
            loop {
                let mut socket = listener.accept().await.unwrap().0;
                println!("accepted");
                h1.spawn(async move {
                    let mut buf = [0; 1000];
                    let count = socket.read(&mut buf).await.unwrap();
                    println!("read {} bytes", count);
                });
            }
        });
        tokio::time::sleep(Duration::from_secs(100000000)).await;
    });
}

In this example, read does not happen, it hangs in accept. (Edit: fixed explanation of what was going on.)

But for simpler example, without spawn of accept loop, it works fine even without forcing listener to be non-blocking. This simpler example does not hang:

use tokio::runtime::Runtime;
use tokio::net::TcpListener;
use tokio::io::AsyncReadExt;
use std::time::Duration;
use tokio::sync::oneshot;

fn main() {
    let reactor = Runtime::new().unwrap();
    let handle = reactor.handle().clone();

    reactor.block_on(async move {
        let listener = std::net::TcpListener::bind(("127.0.0.1:1234")).unwrap();

        let listener = TcpListener::from_std(listener).unwrap();

        println!("listening on port {}", listener.local_addr().unwrap().port());
        let h1 = handle.clone();
        // handle.spawn(async move {
            loop {
                let mut socket = listener.accept().await.unwrap().0;
                println!("accepted");
                h1.spawn(async move {
                    let mut buf = [0; 1000];
                    let count = socket.read(&mut buf).await.unwrap();
                    println!("read {} bytes", count);
                });
            }
        // });
        // tokio::time::sleep(Duration::from_secs(100000000)).await;
    });
}

So this is very easy mistake to make and very hard to debug.

We should

• either assert that the socket is non-blocking
• or just force listener.set_non_blocking(true) in TcpListener::from_std() (and probably other from_std functions too).
• or at least rename TcpListener::from_std function to something like TcpListener::from_std_must_be_non_blocking to make that explicit. Yeah, I see rustdoc says "it is left up to the user to set it in non-blocking mode", but that's too easy to overlook.

[carllerche]

Thanks for reporting this. I am assigning @Darksonn to put together a proposal.

[zeenix]

I bumped into this today and spent the whole afternoon (instead of meeting cool folks at FOSDEM) to figure out why my tasks don't spawn.

Thanks for reporting this. I am assigning @Darksonn to put together a proposal.

@Darksonn polite reminder. :) I think the solution here is simple: since from_std takes ownership of the std listener, just set the non-blocking mode in the methods, instead of requiring it from the user. I don't see a downside to this. Perhaps I'm missing something obvious here?

I can provide a PR for this.

[Darksonn]

The problem is not making the PR — it is coming to a decision about which of the options is best.

In fact, there's already a PR: #5597.

[zeenix]

The problem is not making the PR — it is coming to a decision about which of the options is best.

Gotcha! Given that it's a pretty bad footgun that wastes people's time and that from_std is an advertised workaround for some missing APIs and that it's been 3 years since this was first filed, I think it'd be nice to get to a decision soon.

In fact, there's already a PR: #5597.

Cool. That's pretty complicated and seem to suggest new/breaking API though. I don't really see the use case of having user set the non-blocking mode themselves since tokio requires that.

My suggestion above is simple and wouldn't require API break in most cases. The only hurdle I bumped into while trying to write the PR was that some of the from_std methods are not fallible and setting non-block mode is a fallible operation. Perhaps we could consider this minor breaking change since it would also make the API more consistent (all from_std methods would then be fallible)?

[carllerche]

I think, from the docs on from_std, it is "clear" that Tokio expects a non-blocking socket. While I know there have been cases (years ago) where at least one person explicitly passed a blocking socket to from_std, I do think it was not an original intent of the API and generally a not great idea.

What we could consider doing is:

1. Start by printing out warnings when debug_assertions are on if a blocking socket is passed in. Also, include a link to a tracking issue where anyone that sees the warning can post a comment if they have a legit case for passing in a blocking socket.

2. At some point in the future, switch the warning to a debug assertion.

If we find anybody who still has a legitimate case for passing in a blocking socket, we can consider adding a specific API for that that bypasses the debug assertion.

[zeenix]

If we find anybody who still has a legitimate case for passing in a blocking socket

I don't think anyone was asserting there is a use case for this. The problem is that since the non-blocking setting is not reflected in the type, people can make mistakes and end up wasting hours debugging this. A runtime assertion would help for sure but still not so nice.

What would be a more interesting question, is if there is a good reason tokio can't do this for the user? Given user gives up the ownership of the socket in question, I don't see why tokio can't set any settings on it, that it needs.

[Darksonn]

That could be a reasonable choice if we designed the API today, but I'm concerned about breaking changes. It also introduces a runtime cost, because most sockets passed to from_std will already be in non-blocking mode.

There have been ideas such as deprecating from_std and adding two new alternatives with different names.

[zeenix]

I'm concerned about breaking changes.

That's fair.

It also introduces a runtime cost, because most sockets passed to from_std will already be in non-blocking mode

Is that a significant enough cost to care about? 🤔 This is not an objection. I'm genuinely curious.

There have been ideas such as deprecating from_std and adding two new alternatives with different names.

If you want to not break the API, then those options sound good to me.