Async API bugs and design flaws

[BigBIueWhale]

As it currently stands, it's impossible to use the API of this library as a user without encountering race conditions or even without blocking. This is due to both implementation flaws and design flaws at the API level.

Does get_directory_map block? Should say in the README.

This doesn't make sense:

    let guard = match scanner.directory_map.lock() { ... }
    // Quickly clone the data needed and release the lock.
    let directory_map = (*guard).clone();

given that guard isn't released until the end of the scope (which in this case is the end of the function). Here's a solution to that problem: Fix Rust Lock Scope

There's also a similar issue at scan_directory_async. Because of these lines:

let mut directory_map = directory_map_clone.lock().unwrap();
*directory_map = root_hierarchy;

the directory_map will be locked until the end of the thread execution.

This means that in the current implementation:

1. scan_directory_async spawns a thread that locks directory_map throughout this thread's entire execution.
2. get_directory_map acquires the directory_map lock, therefore blocking until the scan_directory_async thread is done.

Another issue is that it's bad practice to rely on Mutex for long blocking preriods- it's not efficient. Better to use Mutex in conjunction with Condvar (equivalent of std::condition_variable).

Here's a basic overview of how you use a Condvar in Rust:

1. Create a Condvar and a Mutex: First, you need to create a condition variable and a mutex. The mutex is used to provide safe access to the shared state.

   use std::sync::{Mutex, Condvar};
   let pair = (Mutex::new(shared_state), Condvar::new());

2. Lock the Mutex: To access the shared state, a thread must first lock the mutex.

   let (lock, cvar) = &*pair;
   let mut started = lock.lock().unwrap();

3. Wait on the Condition Variable: If the condition you're waiting for is not met, you call wait or wait_timeout on the condition variable. This call will block the current thread until another thread signals on the condition variable. While waiting, the mutex is atomically released.

   while !condition_met(&*started) {
      started = cvar.wait(started).unwrap();
   }

4. Signal the Condition Variable: Another thread can signal the condition variable to wake one or all waiting threads. This is typically done after modifying the shared state in such a way that the condition might now be true.

   // Modify the shared state
   *started = new_state;
   // Wake one waiting thread
   cvar.notify_one();
   // Alternatively, wake all waiting threads
   // cvar.notify_all();

5. Re-acquire the Mutex: When a thread is woken up, it automatically re-acquires the mutex lock before wait returns.

Condition variables represent the ability to block a thread such that it consumes no CPU time while waiting for an event to occur.

Altogether, I see 3 issues in the current implementation that have to do with the async nature of this library:

1. get_directory_map is blocking, so this is not an async API
2. If the user calls scan_directory_async and then quickly calls get_directory_map, the thread::spawn might not be done yet, and therefore the previous scan (an old scan, or an empty directory map) will be returned.
3. Don't rely on mutexes for long blocking periods- it's not efficient- use Condvar if need to block until a condition is met.

My solution would be to redesign your API to be event-based (which is a challenge when dealing with cross-language and cross-runtime).

[AviadCohen24]

Thank you for your comprehensive feedback and for identifying the critical issues in the current implementation of the library. Your insights into the problems with blocking, the misuse of Mutex over extended periods, and the async nature of the API are invaluable.

I've taken your advice seriously and have committed to redesigning the API to be event-based, as you suggested. This approach should mitigate the race conditions and blocking issues inherent in the previous design. I've pushed the initial changes to a new branch called development.

I would be honored if you could join the development process. Your expertise would greatly benefit the project, especially as we navigate through these significant changes. I'm particularly interested in your thoughts on the modified code. Additionally, I'm currently facing a challenge with the test_scan_complex_folder function, which is taking much longer than expected. Any insights or suggestions you might have on addressing this issue would be deeply appreciated.

You can find the development branch here: [https://github.com/AviadCohen24/RustDirectorySniffer/tree/development]. Please feel free to share your thoughts, criticisms, and suggestions. Your collaboration would not only help enhance the quality of the library but also accelerate the development process.

Looking forward to potentially working together.