tokio-rs / tokio

A runtime for writing reliable asynchronous applications with Rust. Provides I/O, networking, scheduling, timers, ...
https://tokio.rs
MIT License
26.9k stars 2.48k forks source link

Inconsistent Error Handling Behavior with `?` operator in Marco #[tokio::main] #6930

Open TomMonkeyMan opened 1 week ago

TomMonkeyMan commented 1 week ago

Version Tokio version:

├── tokio v1.41.0
      └── tokio-macros v2.4.0 (proc-macro)

Rust version:

rustc 1.79.0 (129f3b996 2024-06-10)

Reqwest version:

reqwest v0.11.27

Platform

Darwin 22.5.0 Darwin Kernel Version 22.5.0: Thu Jun  8 22:22:22 PDT 2023; 
root:xnu-8796.121.3~7/RELEASE_X86_64 x86_64

Description

When using the #[tokio::main] macro to define an asynchronous main() function, I've encountered inconsistent behavior regarding error handling when returning std::io::Error as part of a Result that has Box<dyn Error> as its error type. Specifically, in the main() function, it seems necessary to use the ? operator before returning it, otherwise it fails to compile. However, in other async functions, Box::new(std::io::Error) can be returned directly without the ? operator conversion.

For example, in the main() function, the following code is required:

Err(Box::new(std::io::Error::new(
    std::io::ErrorKind::Other,
    "Request failed",
)))?

While in other asynchronous functions, the following code works without the need for ?:

Err(Box::new(std::io::Error::new(
    std::io::ErrorKind::Other,
    "Request failed",
)))

Minimal, Complete, and Verifiable Example (MCVE)

To reproduce the issue, you can use the following minimal example:

Code1:

use reqwest::Client;
use std;

#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
    let client = Client::new();
    let test_url = "https://www.google.com";
    let request = client.get(test_url);
    let response = request.send().await?;

    if response.status().is_success() {
        println!("request succeeded");
        let content = response.text().await?;
        println!("request content {:#?}", content);
        Ok(())
    } else {
        let status = response.status();
        let error_text = response
            .text()
            .await
            .unwrap_or_else(|_| "Failed to read response text".to_string());
        eprintln!("Request failed with status: {} {:#?}", status, error_text);

        Err(Box::new(std::io::Error::new(
            std::io::ErrorKind::Other,
            "Request failed",
        )))?  // This line causes a compile error if `?` are not used (`Box::new()`)
    }
}

Code2:

use reqwest::Client;
use std;

pub async fn test() -> Result<(), Box<dyn std::error::Error>> {
    let client = Client::new();
    let test_url = "https://www.google.com";
    let request = client.get(test_url);
    let response = request.send().await?;

    if response.status().is_success() {
        println!("request succeeded");
        let content = response.text().await?;
        println!("request content {:#?}", content);
        Ok(())
    } else {
        let status = response.status();
        let error_text = response
            .text()
            .await
            .unwrap_or_else(|_| "Failed to read response text".to_string());
        eprintln!("Request failed with status: {} {:#?}", status, error_text);

        Err(Box::new(std::io::Error::new(
            std::io::ErrorKind::Other,
            "Request failed",
        )))
    }
}

#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {
    test().await?;
    Ok(())
}

Notice that in Code1, within the main() function, the error must use the ? operator, while in Code2, the test function, the error can be returned directly without operator ?.

Expected Behavior I expect both functions to require the same treatment for returning errors. Consistent error handling across all async functions, including main(), would make the code more intuitive and easier to maintain.

Possible Solutions -Documentation: Clarify in the documentation whether this is expected behavior and why. -Bug Fix: If it's a bug, fix the behavior so that all async functions, including main, handle return errors consistently.

This issue was also discussed in the community: https://users.rust-lang.org/t/why-must-i-convert-std-error-to-box-dyn-error-in-one-function-but-not-in-another/120141/9

Thank you for looking into this!

Darksonn commented 1 week ago

I guess the issue is that we're not properly informing the closure about the return type...

TomMonkeyMan commented 1 week ago

I guess the issue is that we're not properly informing the closure about the return type...

Thank you for the clarification! It makes sense regarding the closure and return type. I’ll take a closer look and experiment with it a bit more to deepen my understanding of tokio. Appreciate the insights—this is a great learning opportunity for me!

Darksonn commented 1 week ago

If we can fix this in a non-intrusive way, then I'm happy to see a PR for that. But changes to this part of the macro have proven tricky in the past.

TomMonkeyMan commented 1 week ago

Thanks for pointing that out! I fully understand that changes in this area can be tricky. If I come up with any ideas or plans for a potential PR, I’ll be sure to discuss them with you and the other maintainers before making any code changes or submitting anything. I really appreciate your guidance and openness to contributions!

Darksonn commented 1 week ago

You don't have to ask first before creating a PR. Just be prepared that it may (or may not!) be difficult to fix this. We have previously closed several PRs in this part of the codebase because they were incorrect and we couldn't figure out how to fix them.

TomMonkeyMan commented 1 week ago

Haha, thanks for the heads-up! I’ll keep that in mind and do my best.