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

bug: multiple threads using node.set_busy #34

Closed
erhant opened this issue Jun 5, 2024 · 2 comments · Fixed by #51
Closed

bug: multiple threads using node.set_busy #34

erhant opened this issue Jun 5, 2024 · 2 comments · Fixed by #51
Labels
bug Something isn't working

Comments

@erhant
Copy link
Member

erhant commented Jun 5, 2024

We need a thread-safe lock for when two threads try to use set_busy. The current implementation may allow multiple threads to use it at different times.

For example, a synthesis task is received and busy := true is set, but then search task is received (due to an earlier heartbeat response) and now search thread will also set busy as true, whereas it should have been locked by the synthesis thread.

@erhant erhant added the bug Something isn't working label Jun 5, 2024
@erhant
Copy link
Member Author

erhant commented Jun 17, 2024

If #51 is merged, there will only be one thread doing work, so this issue will not be present.

@erhant erhant mentioned this issue Jun 24, 2024
16 tasks
@erhant
Copy link
Member Author

erhant commented Jul 16, 2024

Snippet here for the record:

use parking_lot::RwLock;
use std::sync::Arc;
use tokio_util::task::TaskTracker;

struct BusyStruct {
    pub busy_lock: RwLock<bool>,
}

impl BusyStruct {
    pub fn new() -> Self {
        Self {
            busy_lock: RwLock::new(false),
        }
    }
    /// Returns the state of the node, whether it is busy or not.
    #[inline]
    pub fn is_busy(&self) -> bool {
        *self.busy_lock.read()
    }

    /// Set the state of the node, whether it is busy or not.
    #[inline]
    pub fn set_busy(&self, busy: bool) {
        log::info!("Setting busy to {}", busy);
        *self.busy_lock.write() = busy;
    }
}

/// This test demonstrates that two threads dont wait for each other.
/// We need a separate busy lock for task types, so that heartbeat messages can be
/// repsonded to with the correct task types.
/// Run with:
///
/// ```sh
/// cargo test --package dkn-compute --test threads_test --all-features -- threads_test::test_mutex --exact --show-output
/// ```
#[tokio::test]
#[ignore = "only run this for demonstration"]
async fn test_mutex() {
    let _ = env_logger::try_init();
    let tracker = TaskTracker::new();
    let obj = Arc::new(BusyStruct::new());

    println!("Starting test");

    // spawn a thread
    let obj1 = obj.clone();
    tracker.spawn(tokio::spawn(async move {
        println!("Thread 1 | is_busy: {}", obj1.is_busy());
        println!("Thread 1 | Started");
        obj1.set_busy(true);

        tokio::time::sleep(tokio::time::Duration::from_secs(1)).await;
        obj1.set_busy(false);
        println!("Thread 1 | Finished");
    }));

    // wait a bit
    tokio::time::sleep(tokio::time::Duration::from_millis(250)).await;

    // spawn a thread
    let obj2 = obj.clone();
    tracker.spawn(tokio::spawn(async move {
        println!("Thread 2 | is_busy: {}", obj2.is_busy());
        println!("Thread 2 | Started");
        obj2.set_busy(true);
        tokio::time::sleep(tokio::time::Duration::from_secs(1)).await;
        obj2.set_busy(false);
        println!("Thread 2 | Finished");
    }));

    tracker.close();
    println!("Waiting...");
    tracker.wait().await;

    println!("Done.");
}

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