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

sync: RwLock::downgrade is not correct #2941

Closed
carllerche opened this issue Oct 12, 2020 · 2 comments · Fixed by #2957
Closed

sync: RwLock::downgrade is not correct #2941

carllerche opened this issue Oct 12, 2020 · 2 comments · Fixed by #2957
Labels
A-tokio Area: The main tokio crate C-bug Category: This is a bug. M-sync Module: tokio/sync

Comments

@carllerche
Copy link
Member

I noticed CI can spuriously fail on this test.

The following loom test will fail:

#[test]
fn downgrade() {
    loom::model(|| {
        let lock = Arc::new(RwLock::new(1));

        let n = block_on(lock.write());
        let cloned_lock = lock.clone();
        let handle = thread::spawn(move || {
            let mut guard = block_on(cloned_lock.write());
            *guard = 2;
        });

        let n = n.downgrade();
        assert_eq!(*n, 1);

        assert_eq!(*block_on(lock.read()), 1);

        drop(n);

        handle.join().unwrap();
        assert_eq!(*block_on(lock.read()), 2);
    });
}
@carllerche carllerche added C-bug Category: This is a bug. A-tokio Area: The main tokio crate labels Oct 12, 2020
@Darksonn Darksonn added the M-sync Module: tokio/sync label Oct 12, 2020
@zaharidichev
Copy link
Contributor

zaharidichev commented Oct 12, 2020

Hm, this seems to be related to some race in the batch semaphore. I assume the following test should pass but it fails:

#[test]
fn maybe_race() {
    loom::model(|| {
        let semaphore = Arc::new(Semaphore::new(10));
        semaphore
            .try_acquire(10)
            .expect("try_acquire should succeed; semaphore uncontended");
        let semaphore2 = semaphore.clone();
        let thread = thread::spawn(move || block_on(semaphore2.acquire(10)).unwrap());
       
        semaphore.release(1);
        // this fails as semaphore.available_permits() gives 0 ...
        assert_eq!(1, semaphore.available_permits());
       
        semaphore.release(9);
        thread.join().unwrap();
        assert_eq!(0, semaphore.available_permits());
    })
}

I am keen to take a look at that if I am not stepping on somebody's toes... :)

UPDATE: Scratch the above. The test makes sense and should fail. The problem is elsewhere. Will submit a PR shortly.

@LucioFranco
Copy link
Member

I am keen to take a look at that if I am not stepping on somebody's toes... :)

Go for it

zaharidichev added a commit to zaharidichev/tokio that referenced this issue Oct 14, 2020
Currently when `RwLockWriteGuard::downgrade` the `MAX_READS - 1`
permits are added to the semaphore. When `RwLockWriteGuard::drop`
gets invoked however another `MAX_READS` permits are added. This
results in releasing more permits that were actually aquired when
downgrading a write to a read lock. This is why we need to call
`mem::forget` on the `RwLockWriteGuard` in order to avoid
invoking the destructor.

Fixes: tokio-rs#2941
carllerche pushed a commit that referenced this issue Oct 23, 2020
Currently when `RwLockWriteGuard::downgrade` the `MAX_READS - 1`
permits are added to the semaphore. When `RwLockWriteGuard::drop`
gets invoked however another `MAX_READS` permits are added. This
results in releasing more permits that were actually aquired when
downgrading a write to a read lock. This is why we need to call
`mem::forget` on the `RwLockWriteGuard` in order to avoid
invoking the destructor.

Fixes: #2941
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-tokio Area: The main tokio crate C-bug Category: This is a bug. M-sync Module: tokio/sync
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants