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

cpt #7

Closed
wants to merge 6 commits into from
Closed

cpt #7

wants to merge 6 commits into from

Conversation

matklad
Copy link
Owner

@matklad matklad commented Oct 28, 2020

  • mktemp_d and tests for cp
  • Publish 0.1.7

@matklad
Copy link
Owner Author

matklad commented Oct 28, 2020

bors r+

bors bot added a commit that referenced this pull request Oct 28, 2020
7: cpt r=matklad a=matklad

- mktemp_d and tests for cp
- Publish 0.1.7


Co-authored-by: Aleksey Kladov <[email protected]>
@bors
Copy link
Contributor

bors bot commented Oct 28, 2020

Build failed:

  • publish

@matklad
Copy link
Owner Author

matklad commented Oct 28, 2020

bors r+

bors bot added a commit that referenced this pull request Oct 28, 2020
7: cpt r=matklad a=matklad

- mktemp_d and tests for cp
- Publish 0.1.7


Co-authored-by: Aleksey Kladov <[email protected]>
@bors
Copy link
Contributor

bors bot commented Oct 28, 2020

Build failed:

  • publish

@matklad
Copy link
Owner Author

matklad commented Oct 28, 2020

bors r+

bors bot added a commit that referenced this pull request Oct 28, 2020
7: cpt r=matklad a=matklad

- mktemp_d and tests for cp
- Publish 0.1.7


Co-authored-by: Aleksey Kladov <[email protected]>
@bors
Copy link
Contributor

bors bot commented Oct 28, 2020

Timed out.

@matklad
Copy link
Owner Author

matklad commented Oct 28, 2020

bors r+

bors bot added a commit that referenced this pull request Oct 28, 2020
7: cpt r=matklad a=matklad

- mktemp_d and tests for cp
- Publish 0.1.7


Co-authored-by: Aleksey Kladov <[email protected]>
@bors
Copy link
Contributor

bors bot commented Oct 28, 2020

Timed out.

@matklad
Copy link
Owner Author

matklad commented Nov 13, 2020

bors r+

bors bot added a commit that referenced this pull request Nov 13, 2020
7: cpt r=matklad a=matklad

- mktemp_d and tests for cp
- Publish 0.1.7


Co-authored-by: Aleksey Kladov <[email protected]>
@bors
Copy link
Contributor

bors bot commented Nov 13, 2020

Build failed:

  • publish

@azdavis
Copy link
Collaborator

azdavis commented Feb 27, 2021

I am on macOS, and I took a look.

Seems like running just test_pushenv and test_cp (disabling all other tests) sometimes triggers the deadlock. I investigated in https://github.com/azdavis/xshell/commits/master .

A minimal deadlocking test seems to be https://github.com/azdavis/xshell/commit/410794f823f857f89f65222b55d44af1b106841a . I can't see why this would deadlock though. The test does this:

  • It acquires the read lock
  • Then tries to acquire the write lock but blocks since we still have a read guard as expected
  • Then tries to acquire another read guard, which should work, but instead hangs

@matklad
Copy link
Owner Author

matklad commented Feb 27, 2021

You are my hero @azdavis ! I think this explains the behavior! When implementing a RW lock, one problem to keep in mind is writer's starvation.

If many readers repeatedly read lock and unlock the rw lock, a writer may block indefinitely. For this reason, usually, as long as writer tries to grap a lock, all further attempts of readers to lock will block. So after some time, old readers release their locks, the writer does its job, and the new readers are unlocked.

It seems like linux and windows either don't protect against starvation, or special case recursive locking by the same thread.

@matklad
Copy link
Owner Author

matklad commented Feb 27, 2021

@azdavis are you perhaps interested in fixing this? I think just making your unit test work will do the trick. I think it might be enough to extend the logic in https://github.com/azdavis/xshell/blob/410794f823f857f89f65222b55d44af1b106841a/src/gsl.rs#L44-L46 to cache both read and write lock (currently the bool represents write lock).

Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 27, 2021
clarify RW lock's priority gotcha

In particular, the following program works on Linux, but deadlocks on
mac:

```rust
    use std::{
        sync::{Arc, RwLock},
        thread,
        time::Duration,
    };

    fn main() {
        let lock = Arc::new(RwLock::new(()));

        let r1 = thread::spawn({
            let lock = Arc::clone(&lock);
            move || {
                let _rg = lock.read();
                eprintln!("r1/1");
                sleep(1000);

                let _rg = lock.read();
                eprintln!("r1/2");

                sleep(5000);
            }
        });
        sleep(100);
        let w = thread::spawn({
            let lock = Arc::clone(&lock);
            move || {
                let _wg = lock.write();
                eprintln!("w");
            }
        });
        sleep(100);
        let r2 = thread::spawn({
            let lock = Arc::clone(&lock);
            move || {
                let _rg = lock.read();
                eprintln!("r2");
                sleep(2000);
            }
        });

        r1.join().unwrap();
        r2.join().unwrap();
        w.join().unwrap();
    }

    fn sleep(ms: u64) {
        std::thread::sleep(Duration::from_millis(ms))
    }
```

Context: I was completely mystified by a my CI deadlocking on mac ([here](matklad/xshell#7)), until `@azdavis` debugged the issue. See a stand-alone reproduciton here: matklad/xshell#15
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 27, 2021
clarify RW lock's priority gotcha

In particular, the following program works on Linux, but deadlocks on
mac:

```rust
    use std::{
        sync::{Arc, RwLock},
        thread,
        time::Duration,
    };

    fn main() {
        let lock = Arc::new(RwLock::new(()));

        let r1 = thread::spawn({
            let lock = Arc::clone(&lock);
            move || {
                let _rg = lock.read();
                eprintln!("r1/1");
                sleep(1000);

                let _rg = lock.read();
                eprintln!("r1/2");

                sleep(5000);
            }
        });
        sleep(100);
        let w = thread::spawn({
            let lock = Arc::clone(&lock);
            move || {
                let _wg = lock.write();
                eprintln!("w");
            }
        });
        sleep(100);
        let r2 = thread::spawn({
            let lock = Arc::clone(&lock);
            move || {
                let _rg = lock.read();
                eprintln!("r2");
                sleep(2000);
            }
        });

        r1.join().unwrap();
        r2.join().unwrap();
        w.join().unwrap();
    }

    fn sleep(ms: u64) {
        std::thread::sleep(Duration::from_millis(ms))
    }
```

Context: I was completely mystified by a my CI deadlocking on mac ([here](matklad/xshell#7)), until ``@azdavis`` debugged the issue. See a stand-alone reproduciton here: matklad/xshell#15
@azdavis
Copy link
Collaborator

azdavis commented Feb 27, 2021

I took a look in #16 :D

@azdavis azdavis mentioned this pull request Feb 28, 2021
bors bot added a commit that referenced this pull request Mar 1, 2021
16: Add mktemp_d r=azdavis a=azdavis

- Use POSIX date invocations, don't use `--iso` option
  - This might have been part of what was making tests fail on macOS
  - I ran `cargo t` on my macOS machine and tests failed because macOS's default `date` doesn't have the `--iso` option
- Add changelog entry for 0.1.7, 0.1.8 retroactively
- Fix cp doc
  - It acts like shell's `cp` when `dst` is a directory
- Allow acquiring many read locks on the same thread
  - Update the thread-local cache to track readers as well as the writer
- Doc and test internal `gsl` more, note some caveats
- Add `mktemp_d`, doc it
- Update version and release notes
- Ignore `target` files in vscode

Closes #12 , can also probably close #7 after this.

Co-authored-by: Ariel Davis <[email protected]>
Co-authored-by: Aleksey Kladov <[email protected]>
@bors bors bot closed this in 8c20f82 Mar 1, 2021
@azdavis azdavis deleted the cpt branch March 1, 2021 12:24
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.

2 participants