Functions that modify the environment have undocumented safety requirements

[elomatreb]

The typical usage of calling dotenvy::dotenvy(); to load and setup the programs environment variables is missing certain safety requirements, namely that it must be done before any other threads in the program are spawned. This is documented in the stdlib, and set_var & remove_var are slated to become unsafe in Rust 2024 for this reason.

A common way to violate these requirements is the following program:

#[tokio::main]
async fn main() {
    dotenvy::dotenvy();
}

This expands to the following code:

fn main() {
    let body = async { dotenvy::dotenvy(); };

    #[allow(clippy :: expect_used, clippy :: diverging_sub_expression)]
    {
        return tokio::runtime::Builder::new_multi_thread().enable_all().build().expect("Failed building the Runtime").block_on(body);
    }
}

Notice how the dotenvy call gets moved into an async block and run after the tokio runtime threads have been spawned. This is unsound in theory and practice, since tokio internally accesses the environment prior to this point (to allow configuring the number of worker threads through an env var).

The documentation for functions that modify the environment at least need to reproduce the safety requirements from the stdlib, and very probably should become unsafe to mirror the Rust 2024 developments.

[sonro]

Thank you for raising this!

There will likely be a new dotenvy API before the 2024 edition is stable; so set_var becoming unsafe will certainly influence it.

For now, I will mark the safety documentation as a priority.

[sonro]

This is what I have come up with for the rustdoc.

@allan2 could you please review it?

Safety

Even though this function is currently not marked as unsafe, it will
be when the Rust 2024 edition is stabilized. This is tracked in
rust#27970.

This function is safe to call in a single-threaded program. Also for any
program running on Windows.

In multi-threaded programs, you must ensure there are no other threads
concurrently writing or reading(!) from the environment. You are
responsible for figuring out how to achieve this, but we strongly
suggest only using dotenvy before spawning any threads (this includes
the use of tokio::main).

[sonro]

Unfortunately this is going to apply to every public function as they all internally call set_var. Particular care will be needed with the _iter functions. They are safe until Iter::load or Iter::load_override is called on the returned Iter type.

[sonro]

Proposed README addition:

---

Safety

The majority of dotenvy's existing API will be marked as unsafe when the 2024 Rust edition is stabilized. The functions modify the environment, which is considered unsafe in a multi-threaded context on Unix based machines. It is up to the user to ensure that they are using dotenvy before spawning any threads (this includes inside a tokio::main). See the tracking issue for more information.

dotenvy's next release will include a new API which will not require unsafety.

[elomatreb]

On the Rust community discord, @5225225 suggested the idea of a #[tokio::main]-style attribute macro that inserts the necessary call to the unsafe load function as the very first thing of the main function, which would allow users to avoid exposure to unsafe directly if desired.

[elomatreb]

This safety concern also does not apply to Windows, which apparently has thread-safe environment variables.

[sonro]

On the Rust community discord, @5225225 suggested the idea of a #[tokio::main]-style attribute macro that inserts the necessary call to the unsafe load function as the very first thing of the main function, which would allow users to avoid exposure to unsafe directly if desired.

I actually am growing to like this idea.

Example

#[dotenvy::main]
fn main() -> Result<(), AppError> {
    tokio_main()
}

#[tokio::main]
async fn tokio_main() -> Result<(), AppError> {
    // app goes here
    // ...
    Ok(())
}

[allan2]

This is a very important issue that will change the future of dotenvy.

The next release of dotenvy will offer both a safe API (which does not modify the environment) and an unsafe API (which does modify the environment via set_var).

I also really like the attribute macro. It is the most ergonomic way to call the loading function before initiating the multi-threaded runtime.

#[dotenvy::load(config = "tbd")]
#[tokio::main]
async fn main() {
    foo();
}

[sonro]

I think we should add a safety notice now for the existing API. @allan2 would you be OK with me adding the rustdoc and readme additions above?

[tbu-]

Even though this function is currently not marked as unsafe, it will be when the Rust 2024 edition is stabilized.

Note that the unsafety is unrelated to Rust 2024. It's just as unsafe in Rust 2015, 2018, 2021, it's just missing the unsafe marker. The safety discussion on the set_var API existed before making the function officially unsafe.

[allan2]

There is a new API in the v0.16 branch.

let loader = EnvLoader::from_path(path).sequence(EnvSequence::FilesThenEnv);

// does not modify environment
let env_map: HashMap<String, String> = loader.load()?;

 // modifies environment using `set_var`
let env_map: HashMap<String, String> = unsafe { loader.load_and_modify() }?;

There is also an attribute macro that is thread-safe.

// this loads from the file into the environment before the async runtime is started
#[dotenvy::load]
#[tokio::main]
async fn main() {
    println!("inside async runtime");
}

There are examples for tokio without macro and tokio with macro.

So there is progress on this front. Doc still have to be added though, so I will leave this issue open.