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

Having publisher inside the matching_listener callback will cause deadlock #1560

Closed
evshary opened this issue Oct 23, 2024 · 1 comment · Fixed by #1583
Closed

Having publisher inside the matching_listener callback will cause deadlock #1560

evshary opened this issue Oct 23, 2024 · 1 comment · Fixed by #1583
Assignees
Labels
bug Something isn't working

Comments

@evshary
Copy link
Contributor

evshary commented Oct 23, 2024

Describe the bug

Sometimes users might want to publish data inside the matching_listener callback. However, it might cause the deadlock.
While dropping the publisher, we will clean up the matching_listener. But if matching_listener holds a copy of publisher, the drop will not be triggered.
The only way here is to drop matching_listener explicitly, but I'm wondering there is a better solution to deal with this.

To reproduce

Here is the example:

use std::sync::Arc;
use zenoh::{key_expr::KeyExpr, Wait};

#[tokio::main]
async fn main() {
    let session = zenoh::open(zenoh::Config::default()).await.unwrap();
    let publisher = Arc::new(
        session
            .declare_publisher(KeyExpr::new("abc").unwrap())
            .await
            .unwrap(),
    );
    let matching_listener = publisher
        .matching_listener()
        .callback({
            let p = publisher.clone();
            move |_status| {
                println!("abc");
                p.put("payload").wait().unwrap();
            }
        })
        .await
        .unwrap();
    // If we don't undeclare explicitly, the process will be stuck (deadlock)
    //matching_listener.undeclare().await.unwrap();
    println!("Should exit successfully....");
}

Note that if we call matching_listener.undeclare().await.unwrap();, then everything goes well.

System info

  • Platform: Ubuntu 24.04
@evshary
Copy link
Contributor Author

evshary commented Oct 29, 2024

Another way is to use Weak inside the callback, but users still need to take care of it by themselves.

use std::sync::Arc;
use zenoh::{key_expr::KeyExpr, Wait};

#[tokio::main]
async fn main() {
    let session = zenoh::open(zenoh::Config::default()).await.unwrap();
    let publisher = Arc::new(
        session
            .declare_publisher(KeyExpr::new("abc").unwrap())
            .await
            .unwrap(),
    );
    let matching_listener = publisher
        .matching_listener()
        .callback({
            let p = Arc::downgrade(&publisher);
            move |_status| {
                println!("abc");
                p.upgrade().unwrap().put("payload").wait().unwrap();
            }
        })
        .await
        .unwrap();
    println!("Should exit successfully....");
}

I wonder whether there is an easy way to automatically avoid the cyclic reference between Publisher and MatchingListener.

  • No reference from MatchingListener back to Publisher:
    • Unable to pass Arc<Publisher> inside the callback, which is not possible
    • Provide a weak Publisher inside callback to prevent users from doing so by themselves
  • No reference from Publisher to MatchingListener:
    • MatchingListener can be released separately, but this will increase the complexity of the API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant