1
\$\begingroup\$

I'm really new to Rust with less than a day of rusting. I'd like to know if the following code is ok or could it be written somehow better, and specifically, which would be the preferred way to handle the actual error case.

The return type bool represents whether a packet was received or not, so that the call to tryReceiveUdp can be chained using bool::then to do something else instead.

fn tryReceiveUdp(socket:UdpSocket) -> std::io::Result<bool>
{
    let mut buf = [0; 1024];
    
    let r = match socket.recv_from(&mut buf)
    {
        Ok((bytes,src)) => {
            handleUdp(buf[..bytes].to_vec(),src)?;
            false
        },
        Err(e) if e.kind() == std::io::ErrorKind::WouldBlock => true,

        //Err(e) => Err(e)  <-- Is this better than the next line?
        e => {e?;true}
    };
    Ok(r)
}

Edit: There's a mistake in the above. The code initially didn't have Ok(r) at the end. Instead the specific question was between these two

Err(e) => Err(e),  //<-- Is this better than the next line?
e => {e?;Ok(true)},
\$\endgroup\$
8
  • 1
    \$\begingroup\$ Returning true when nothing happened is very unintuitive for a try* function. \$\endgroup\$ Commented Feb 2, 2024 at 19:58
  • \$\begingroup\$ @kmdreko Yes, that seemed a bit questionable to me too. The idea is to do essentially this in a loop: tryReceiveUdp(&socket)?.then(|| sleep(Duration::from_millis(100))); \$\endgroup\$ Commented Feb 2, 2024 at 21:03
  • \$\begingroup\$ That doesn't seem like a good reason to me, nothing wrong with if !tryReceiveUdp(&socket)? { sleep(Duration::from_millis(100)) } \$\endgroup\$ Commented Feb 2, 2024 at 21:32
  • \$\begingroup\$ @kmdreko I'll eventually have more steps. The idea is that the loop would sleep only when nothing else is done. In C I'd just have a bool variable to indicate whether to sleep or not and check that at the end of the loop. I'm wondering what is the most rustic approach. \$\endgroup\$ Commented Feb 2, 2024 at 22:14
  • 1
    \$\begingroup\$ @RichardNeumann Yes, that is a bit sloppy. The idea is that it's large enough. I'll be receiving fixed size messages and I'll be able to check at compile time that the messages fit. I suppose I could figure out how to get the max size over all of the message types as a compile time constant. \$\endgroup\$ Commented Feb 6, 2024 at 8:54

1 Answer 1

Reset to default
6
\$\begingroup\$

The path I would take with the error processing in this code would be to remove the let r = ...; Ok(r) and have the match compute the return value directly:

fn tryReceiveUdp(socket:UdpSocket) -> std::io::Result<bool>
{
    let mut buf = [0; 1024];
    
    match socket.recv_from(&mut buf)
    {
        Ok((bytes,src)) => {
            handleUdp(buf[..bytes].to_vec(),src)?;
            Ok(false)
        },
        Err(e) if e.kind() == std::io::ErrorKind::WouldBlock => Ok(true),
        Err(e) => Err(e),
    }
}

This has more duplicated Oks, but it is easy to see at a glance whether each case is either a success or a failure.

As to the exact choice of error propagation operation Err(e) => Err(e): I generally prefer not to use the ? operator with a value known to be an error. In this case, it's simple to not early-return at all and just let the match pass the Err out, but in the case where an early-return is necessary, I'd still rather write return Err(e); or return result; rather than using an ? that appears to unconditionally return. I avoid that use of ? for several reasons:

  • it's potentially confusing to readers,
  • it contains a pointless branch that can never be taken (though that will of course be optimized out), and
  • it requires you to write something after it that type-checks as if control flow could continue past that point (the ;true in your code).

Other things I notice in review:

  • Your function takes the socket by value, so you'll only be able to receive one packet. Consider taking it by reference instead (&UdpSocket).
  • Consider importing modules to use shorter paths rather than repeating std:: everywhere.
  • Rust functions should be named with snake_case, not camelCase.
  • Use standard code formatting, as performed by cargo fmt.

With all of those changed:

use std::io;
use std::net::UdpSocket;

fn try_receive_udp(socket: &UdpSocket) -> io::Result<bool> {
    let mut buf = [0; 1024];

    match socket.recv_from(&mut buf) {
        Ok((bytes, src)) => {
            handle_udp(buf[..bytes].to_vec(), src)?;
            Ok(false)
        }
        Err(e) if e.kind() == io::ErrorKind::WouldBlock => Ok(true),
        Err(e) => Err(e),
    }
}
\$\endgroup\$
6
  • \$\begingroup\$ I hadn't set up the loop calling this function yet, so the compiler didn't complain about the issue with socket move semantics. Good catch! I take it that you'd prefer the Err(e) => Err(e) over the alternative? \$\endgroup\$ Commented Feb 2, 2024 at 20:27
  • \$\begingroup\$ In C++, it's usually recommended to avoid using. And in Java & Scala it's preferred to import only needed items. In this case, there is std::Result, which is now shadowed by std::io::Result. Is Rust somehow different in this respect? \$\endgroup\$ Commented Feb 2, 2024 at 20:28
  • 2
    \$\begingroup\$ @TrayMan this code doesn't shadow Result, it only brings io into scope. \$\endgroup\$ Commented Feb 2, 2024 at 20:47
  • \$\begingroup\$ @kmdreko I see. So use std::io is not like using std::io in C++, or import std.io.* in Java. \$\endgroup\$ Commented Feb 2, 2024 at 20:54
  • \$\begingroup\$ @TrayMan It would in fact shadow Result if you wrote use std::io::*; or use std::io::Result; But the only time use will bring in a name you didn't write explicitly is when you write ::* (glob import). \$\endgroup\$ Commented Feb 3, 2024 at 0:54

You must log in to answer this question.

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.