fix(services/memcached): TCP address resolution

[flip1995]

Which issue does this PR close?

PR #7112 added support for Unix sockets to the memcached backend. As a side effect, it switched the TCP address parsing from being done inside of TcpStream::connect to SocketAddr::parse. The major difference is that SocketAddr::parse cannot resolve addresses like localhost:1234, as it errors out if it sees non-octal numbers in the address.

I didn't create an issue, but can do so if required.

cc @zenyanle @tisonkun

Rationale for this change

#7112 claimed:

Existing TCP configurations (e.g., 127.0.0.1:11211) remain unaffected.

But it broke TCP configurations like tcp://localhost:11211.

So, this seemed like an unintended breaking change to me. I also can't see the benefit of doing that, as TcpStream::connect already calls to_socket_addrs internally.

What changes are included in this PR?

Remove the additional and more restrictive SocketAddr parsing.

Are there any user-facing changes?

This restores the behavior from before the 0.56.0 release. This is required to bump the opendal dependency in sccache:

• Bump opendal to 0.56.0 and fix fallout (fixes local GCS cache usage) mozilla/sccache#2715,

which fixes a transitive GCS issue that was fixed in opendal in version 0.56.0:

• Tracking issues for reqsign-core v2 #6553

AI Usage Statement

I used Gemini to find the PR #7112 that introduced this change, but came up and implemented the fix myself.

[flip1995]

Both of those CI failures seem unrelated to this PR.

[flip1995]

Nice, CI is passing now 🎉

Friendly ping @Xuanwo, could you take a look at this please? This is blocking a fix in sccache for the GCS backend.

[flip1995]

Bump @Xuanwo: This is a really small 5 line PR, can you please take a look?

[erickguan]

Looks good. Can we add regression tests?

Something similar to:

#[cfg(test)]
mod tests {
    use super::*;
    use tokio::net::TcpListener;

    #[tokio::test]
    async fn connect_tcp_socket() -> std::io::Result<()> {
        # test a few other use cases
        let listener = TcpListener::bind("127.0.0.1:0").await?;
        let addr = listener.local_addr()?.to_string();

        let accepted = tokio::spawn(async move {
            let _ = listener.accept().await?;
            Ok::<_, std::io::Error>(())
        });

        let _stream = SocketStream::connect_tcp(&addr).await?;
        accepted.await.unwrap()?;

        Ok(())
    }

    #[cfg(unix)]
    #[tokio::test]
    async fn connect_unix_socket() -> std::io::Result<()> {
        use std::time::{SystemTime, UNIX_EPOCH};
        use tokio::net::UnixListener;

        # can use tempfile instead.
        let path = std::env::temp_dir().join(format!(
            "opendal-memcached-{}.sock",
            SystemTime::now()
                .duration_since(UNIX_EPOCH)
                .unwrap()
                .as_nanos()
        ));

        let listener = UnixListener::bind(&path)?;

        let accepted = tokio::spawn(async move {
            let _ = listener.accept().await?;
            Ok::<_, std::io::Error>(())
        });

        let _stream = SocketStream::connect_unix(path.to_str().unwrap()).await?;
        accepted.await.unwrap()?;

        let _ = std::fs::remove_file(path);
        Ok(())
    }
}

[flip1995]

Good idea, I added regression tests for IpV4, IpV6 and the localhost version that was fixed by this PR, as well as the unix_socket test you suggested.

There's no direct dependency on tempfile yet, so I used the env::temp_dir.

[erickguan]

Some final comment updates and we are good to go!

[flip1995]

Thanks for getting back to this quickly! Improved the comments, as you suggested.

[erickguan]

Thank you!

[flip1995]

Thanks for the review 🙏

Do you have a release schedule? Or are you just releasing whenever enough features/fixes have accumulated?

[dentiny]

I usually don't use hard-coded port number -- 11211 seems to be a well-known port number for memcached.

[flip1995]

I think this is fine in a test case. And all other tests I saw here and in sccache use the hard-coded 11211 port number.