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

Shutdown unix streams before dropping #237

Merged
merged 3 commits into from
Oct 31, 2024

Conversation

A6GibKm
Copy link
Collaborator

@A6GibKm A6GibKm commented Sep 21, 2024

No description provided.

@A6GibKm
Copy link
Collaborator Author

A6GibKm commented Sep 21, 2024

cc @SeaDve this fixes the issue in Bustle 0.9.

@SeaDve
Copy link
Contributor

SeaDve commented Sep 22, 2024

cc @SeaDve this fixes the issue in Bustle 0.9.

Interesting. Still, there's probably an issue somewhere, causing bustle/zbus to freeze but not bustle/dbus-monitor.

@A6GibKm
Copy link
Collaborator Author

A6GibKm commented Sep 22, 2024

I think shutdown will write the eof byte and we might be just waiting forever if it was not set. What i dont follow is that close should implicitly call shutdown.

let mut x1 = {
let (x1, x2) = UnixStream::pair()?;
proxy.retrieve(&x2).await?;
x2.shutdown(Shutdown::Both)?;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Afaik, there is no need to close both halves, closing the writing half should suffice.

@A6GibKm
Copy link
Collaborator Author

A6GibKm commented Sep 27, 2024

@SeaDve @bilelmoussaoui Could you please verify that this actually fixes the issue?

The reproducer is to call the retrieve method while bustle 0.9 (note that flathub is at 0.10) is recording the session bus.

@bilelmoussaoui
Copy link
Owner

@SeaDve @bilelmoussaoui Could you please verify that this actually fixes the issue?

The reproducer is to call the retrieve method while bustle 0.9 (note that flathub is at 0.10) is recording the session bus.

I have it on my todo list. will try to have a look the upcoming days

Fixes an issue where recording with bustle would block ashpd's retrieve
secret method.
This is more general. Since BorrowedFd implements AsFd this should not
really break users.
gnome-keyring will always pass 64 bytes so we allocated only once. If
other implementations require:

 - less: Then we still allocate fewer times, and extra bytes do not harm
 - more: Then we still allocate fewer times than before
@A6GibKm A6GibKm changed the title WIP: Shutdown unix streams before dropping Shutdown unix streams before dropping Oct 31, 2024
@A6GibKm
Copy link
Collaborator Author

A6GibKm commented Oct 31, 2024

Can confirm that this fixes the issues when tested with ASHPD demo (async-std) and Bustle on the commit before migrating to dbus-monitor and without this MR it does not work.

@bilelmoussaoui bilelmoussaoui merged commit c14a742 into bilelmoussaoui:master Oct 31, 2024
10 checks passed
@bilelmoussaoui bilelmoussaoui deleted the test-zbus branch October 31, 2024 20:57
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