Clarify that RUST_MIN_STACK may be internally cached

[seanlinsley]

For larger applications it's important that users set RUST_MIN_STACK at the start of their program because min_stack caches the value. Not doing so can lead to their env::set_var call surprisingly not having any effect.

In my own testing RUST_MIN_STACK had no effect until I moved it to the top of main(). Hopefully this clarification in the docs will help others going forward.

[rustbot]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @m-ou-se (or someone else) soon.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

[workingjubilee]

It's not clear that we should not be not-caching it instead.

[ChrisDenton]

With environment variables like these we don't make any promises since the intent is for users (or other, external, tools) to set them. They aren't intended as an internal API.

It's possible that in the future RUST_MIN_STACK could be cached before main (heck it's even possible that the entire environment block could be frozen on some platforms). Conversely it's also possible we don't cache them at all.

Making strong promises here would be a libs-api decision. But personally I think that if we want to support a global override then we should have a proper API for that rather than messing with the environment.

[seanlinsley]

I think it's reasonable to expose RUST_MIN_STACK as a global setting similar to #[global_allocator]. But in the meantime, this PR at least clarifies the current behavior.

[workingjubilee]

Can we note it down in a more "the check on this environment variable shouldn't be relied on to happen at a particular moment, and that's partly because currently we cache it as an optimization" way, then, so this doesn't get construed as a promise?

[ChrisDenton]

I am still super nervous about mentioning implementation details that shouldn't be relied upon. We tend to avoid that.

We could document that setting RUST_MIN_STACK once the program is running may not have any effect and instead to use Builder::stack_size. This would alert the user to potentially surprising behaviour and the "may" gives enough wriggle room in the implementation.

[rustbot]

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

• Stabilizing library features
• Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
• Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
• Changing public documentation in ways that create new stability guarantees
• Changing observable runtime behavior of library APIs

[seanlinsley]

Okay, I've changed it to:

Be aware that changes to RUST_MIN_STACK may be ignored after program start.

[ChrisDenton]

That seems reasonable to me.

@bors r+

[bors]

📌 Commit 6eef755 has been approved by ChrisDenton

It is now in the queue for this repository.