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 invalid response id after closing file #77

Merged
merged 4 commits into from
May 13, 2023
Merged

fix invalid response id after closing file #77

merged 4 commits into from
May 13, 2023

Conversation

silver-ymz
Copy link
Member

Sometimes after closing file, the server will send success message later. At this time, if we have removed response_id from SharedData, it will raise the error of invalid_response_id.

Signed-off-by: silver-ymz <yinmingzhuo@gmail.com>
@NobodyXu
Copy link
Member

NobodyXu commented May 13, 2023

Thank you for the PR!

This is an short sight in the openssh-sftp-client, where I didn't think of this.

There's actually another, probably better fix for this:

In openssh-sftp-client, src/handle.rs, it drops the id right after sending a close request.

This is the culprit, you could change the Drop to wait for the close request by using tokio::spawn.

It will infer additional cost but IMHO might be better than ignoring any id-not-found related STATUS response.

Edit:

Change the drop impl for OwnedHandle to something like this:

let write_end = &mut self.write_end;
let handle = &self.handle;

if Arc::strong_count(handle) == 1 {
    // This is the last reference to the arc
    let id = write_end.get_id_mut();
    if let Ok(response) = write_end.send_close_request(id, Cow::Borrowed(handle)) {
        // Requests is already added to write buffer, so wakeup
        // the `flush_task`.
        self.get_auxiliary().wakeup_flush_task();
        let future = response.wait();
        tokio::spawn(async move {
            // Ignore the result so that the tokio task does not allocate space for the Result<(Id, ()), Error>
            let _ = future.await;
        );
    }
}

Signed-off-by: silver-ymz <yinmingzhuo@gmail.com>
@NobodyXu
Copy link
Member

@silver-ymz Can you please test this against the PR in opendal?

@NobodyXu
Copy link
Member

BTW, @silver-ymz do you want to become a maintainer of this crate?

Copy link
Member

@NobodyXu NobodyXu left a comment

Choose a reason for hiding this comment

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

And please update the doc for "mod unreleased" in src/changelog.rs

Signed-off-by: silver-ymz <yinmingzhuo@gmail.com>
@silver-ymz
Copy link
Member Author

@silver-ymz Can you please test this against the PR in opendal?

I will begin to make commit in opendal soon.

BTW, @silver-ymz do you want to become a maintainer of this crate?

Thank you for giving me this chance. I'd like to become a maintainer. Sometimes because of my studies I may not be able to deal with issues in time, but I will try to maintain it as much as possible.

Signed-off-by: silver-ymz <yinmingzhuo@gmail.com>
@NobodyXu
Copy link
Member

I will begin to make commit in opendal soon.

#thumb_up

Thank you for giving me this chance. I'd like to become a maintainer.

I've invited you to become a maintainer of openssh-sftp-client, openssh-sftp-protocol, concurrent_arena and awaitable github repository.

@silver-ymz Please also provide me with your crates.io account name.

Also, if you'd like, I can ask jonhoo to invite you to the openssh-rust org, so that you can contribute to every repository under it.

Sometimes because of my studies I may not be able to deal with issues in time, but I will try to maintain it as much as possible.

That's not a problem, everyone has their own life, just enjoy and contribute when you have time.

Having another maintainer will also reduce the bus-factor of this project while also making updates/improvements more frequent/easier, and all of this is volunteer work/hobbies, I am actually grateful that you are willing to step up and become a maintainer.

@NobodyXu NobodyXu merged commit 8a1e0cb into openssh-rust:main May 13, 2023
@NobodyXu
Copy link
Member

Will do a release later today.

@silver-ymz silver-ymz deleted the fix/invalid-response-id branch May 13, 2023 12:14
@NobodyXu
Copy link
Member

@jonhoo Can you please invite @silver-ymz to the openssh-rust org so that they have rw access (or even maintainer access) to all crates?

Thank you!

@jonhoo
Copy link

jonhoo commented Aug 6, 2023

@NobodyXu invited! 🎉

@NobodyXu
Copy link
Member

NobodyXu commented Aug 6, 2023

Thank you!

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

Successfully merging this pull request may close these issues.

3 participants