Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix bug: Async client may not close #236

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jokemanfire
Copy link
Contributor

when just new a Client,the fd will increase.
I also think the lifetime in async client is too long. also in sync #225 .

I obtained the following test code for this phenomenon:

fn get_describe_numbers(pid: &str) -> i64 {
    let proc_dir = format!("/proc/{}/fd", pid);
    let mut cnt = 0;
    match read_dir(PathBuf::from(proc_dir)) {
        Ok(entries) => {
            for entry in entries {
                match entry {
                    Ok(e) => {
                        cnt += 1;
                    }
                    Err(e) => {
                        eprintln!("Error reading directory entry: {}", e);
                    }
                }
            }
        }
        Err(e) => {
            eprintln!("Error reading directory: {}", e);
        }
    }
    cnt
}

#[cfg(unix)]
#[tokio::main(flavor = "current_thread")]
async fn main() {
    let pid = process::id().to_string();
    let pre_fds = get_describe_numbers(pid.as_str());
    for i in 0..10 {
        let now_fds = get_describe_numbers(pid.as_str());
        if now_fds != pre_fds {
            println!("fd is not release pre_fd {pre_fds:?}  now_fd {now_fds:?}");
            // pre_fds = now_fds
        }
        println!(
            "-------------now fd number is {now_fds:?}---------------------------------------"
        );
        let c = Client::connect(utils::SOCK_ADDR).unwrap();
    }
    let now_fds = get_describe_numbers(pid.as_str());
    if now_fds != pre_fds {
        println!("fd is not release pre_fd {pre_fds:?}  now_fd {now_fds:?}");
        // pre_fds = now_fds
    }

    println!("over");
}

when just new a Client,the fd will increase.
@@ -45,6 +46,7 @@ impl Client {
pub fn new(fd: RawFd) -> Client {
let stream = utils::new_unix_stream_from_raw_fd(fd);
Copy link
Member

Choose a reason for hiding this comment

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

The fd will be closed when ClientClose dropping

Copy link
Contributor Author

Choose a reason for hiding this comment

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

hi, It looks like async client didn't hold ClientClose? If I miss?

Copy link
Collaborator

@wllenyj wllenyj left a comment

Choose a reason for hiding this comment

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

The Connection holds the fd. Did not the fd close when conn was dropped?

        let conn = Connection::new(stream, delegate);
        tokio::spawn(async move { conn.run().await });

@jokemanfire
Copy link
Contributor Author

jokemanfire commented Sep 26, 2024

let conn = Connection::new(stream, delegate);

I think connection is client's member, the lifetime of connection should not above the client struct, if client is drop as well as conn should exit. If conn is lock may cause thread leak.

@wllenyj
Copy link
Collaborator

wllenyj commented Sep 27, 2024

I am not sure, is not the fd closed by the Connection. If true, there is now a risk of a double close.
If any fd is opened between two closes, it will be closed silently.

@jokemanfire
Copy link
Contributor Author

jokemanfire commented Sep 27, 2024

I am not sure, is not the fd closed by the Connection. If true, there is now a risk of a double close. If any fd is opened between two closes, it will be closed silently.

I check the connection , but there's no fd release, please check it again if i missed.

@wllenyj wllenyj self-requested a review October 2, 2024 02:59
@jokemanfire
Copy link
Contributor Author

jokemanfire commented Oct 11, 2024

Raw fd looks like difficulty to manage ownership and lifecycle how about use this https://docs.rs/tokio/latest/tokio/net/struct.UnixStream.html. If it is reasonable , I'd like to change it . @wllenyj @Tim-Zhang

OwnedFd may be greater than Rawfd

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants