Skip to content

fix(services/memcached): TCP address resolution#7701

Open
flip1995 wants to merge 1 commit into
apache:mainfrom
flip1995:tcp-address-resolve-fix
Open

fix(services/memcached): TCP address resolution#7701
flip1995 wants to merge 1 commit into
apache:mainfrom
flip1995:tcp-address-resolve-fix

Conversation

@flip1995

@flip1995 flip1995 commented Jun 5, 2026

Copy link
Copy Markdown

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:

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

AI Usage Statement

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

@flip1995 flip1995 requested a review from Xuanwo as a code owner June 5, 2026 10:58
@dosubot dosubot Bot added size:XS This PR changes 0-9 lines, ignoring generated files. releases-note/fix The PR fixes a bug or has a title that begins with "fix" labels Jun 5, 2026
@flip1995 flip1995 changed the title [memcached] Fix TCP address resolution fix(memcached): TCP address resolution Jun 5, 2026
@flip1995 flip1995 changed the title fix(memcached): TCP address resolution fix(services/memcached): TCP address resolution Jun 5, 2026
@flip1995

flip1995 commented Jun 5, 2026

Copy link
Copy Markdown
Author

Both of those CI failures seem unrelated to this PR.

@flip1995 flip1995 force-pushed the tcp-address-resolve-fix branch from 38488ef to 8f7ee39 Compare June 8, 2026 17:48
PR apache#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.

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.
@flip1995 flip1995 force-pushed the tcp-address-resolve-fix branch from 8f7ee39 to 0cfaa3c Compare June 9, 2026 17:37
@flip1995

Copy link
Copy Markdown
Author

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

Copy link
Copy Markdown
Author

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

@erickguan erickguan left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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(())
    }
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

releases-note/fix The PR fixes a bug or has a title that begins with "fix" size:XS This PR changes 0-9 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants