Skip to content

Implement task-safe "current directory" #10463

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
klutzy opened this issue Nov 13, 2013 · 12 comments
Closed

Implement task-safe "current directory" #10463

klutzy opened this issue Nov 13, 2013 · 12 comments

Comments

@klutzy
Copy link
Contributor

klutzy commented Nov 13, 2013

Currently we provide os::getcwd()/os::change_dir() via OS functions (libc::getcwd or GetCurrentDirectory), but curdir information is maintained per-process, which means it is not thread-safe nor task-safe.

Here is a sample code for current dir race:

use std::os;
use std::io;
use std::io::fs;

fn main() {
    let tmp_path = os::tmpdir();
    for i in range(0u, 20u) {
        let path = tmp_path.join(i.to_str());
        do spawn {
            io::result(|| fs::mkdir(&path, io::UserRWX));
            let _ret = os::change_dir(&path);
            let cur = os::getcwd();
            if cur != path {
                println!("expected {:s} but found {:s}",
                          path.as_str().unwrap(), cur.as_str().unwrap());
            }
        }
    }
}

So we must ban them and implement "current directory" using task-local storage.

@alexcrichton
Copy link
Member

I like this idea! I'd want to make sure it could actually be supported though. Right now the main use cases for a "working directory" that I can think of are:

  • change_dir - this can definitely become task-local
  • process spawning - we can make this task-local by always specifying the new cwd (never let it be null)
  • making paths absolute - this is already defined in rust, so we can make this use the task-local current directory

I believe that we can make a transition to a task-level current-working directory with those use cases. That being said, we shouldn't throw away the ability to change the os-level current working directory. Perhaps unsafe fn os::raw::setcwd(p: &Path) or something like that.

@ghost
Copy link

ghost commented Nov 13, 2013

Does the same apply to getenv/setenv? No race there, but task-level envvars could be useful.

@klutzy
Copy link
Contributor Author

klutzy commented Nov 14, 2013

@Jurily There is similar race on os::setenv + libc. For example extra::time::at() (internally uses localtime()) result depends on os::setenv("TZ", "...");.

@lifthrasiir
Copy link
Contributor

If we have the current directory as a task-local property, then we should rename os::getcwd() and os::change_dir() to os::task_cwd() and os::set_task_cwd() respectively. (This naming is along the lines of rand::task_rng(), for example.) I personally don't want to keep the original task-unsafe functions, since users would use them via unsafe blocks. Anyone should use such task-unsafe functions may use libc functions instead.

@vadimcn
Copy link
Contributor

vadimcn commented May 9, 2014

What about external libs? They wouldn't know about Rust's task-local cwd.
Unfortunately, the right solution here is probably just to treat the current directory as immutable (value being whatever it was set to when the current process got started).

@aturon
Copy link
Member

aturon commented Jun 2, 2014

I'm wondering about the coverage of the use-cases that @alexcrichton outlined. The cwd is essentially a bit of process-level state that any system call might use. It's not obvious that we can make it task-local while retaining the right semantics for system calls.

For example, we have a lot of functions in modules like std::io::fs that take Path values but pass their string data down to an underlying system call without turning them into absolute paths. The underlying system calls will then use the process-level cwd. Presumably, we would instead want the task-local cwd to be used, but that may be difficult in general:

  • We'd need to make all paths absolute using the task-local cwd before invoking syscalls. That's probably OK, though we'd have to be careful to ensure that doing so doesn't change the semantics.
  • More worrying, do we always know when something is a path? If we ever pass a string down to a syscall that interprets it as a cwd-relative path, without making it absolute ourselves, we'd be in trouble. One place this happens is in process spawning, but there we could set the process-level cwd. But there may be other places, too.
  • Are the system calls for which the process-level cwd is important, where there's no way to provide an absolute path (say, because they are not taking any path as input)?

@thestinger
Copy link
Contributor

It's incorrect to implement a task-local directory with a Path because a directory can be moved around or something else can be mounted on top of that tree. It would need to use the openat family of functions.

@aturon aturon added the A-io label Jun 8, 2014
@l0kod
Copy link
Contributor

l0kod commented Nov 11, 2014

cc #15643

@steveklabnik
Copy link
Member

As part of IO reform, we got current_dir, but its implementation is os_imp::getcwd, so not much has changed.

@aturon
Copy link
Member

aturon commented Feb 16, 2015

@steveklabnik

I think this ticket should move to the RFCs wishlist. What we wound up with in IO reform is basically exposing the system APIs, which work with process-global working directories. On the posix side at least there are at variants that we may want to expose eventually, but this would be a new API surface.

@l0kod
Copy link
Contributor

l0kod commented Mar 1, 2015

this would be a new API surface

It may be possible to replace most Path with a common trait to Path and File (e.g. AsIo).
cc #21936

@alexcrichton
Copy link
Member

I'm going to close this in favor of a future to-be-opened RFC issue, but it's unlikely that we'll do this with today's design principles of the standard library (e.g. not adding our own abstractions on top)

flip1995 pushed a commit to flip1995/rust that referenced this issue Mar 10, 2023
Add `let_with_type_underscore` lint

Fixes rust-lang#10463
changelog: [`let_with_type_underscore`]: Add the lint.
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

8 participants