[std.process] Make environment use a ReadWriteMutex

[ntrel]

Fixes #10580.

Make getEnvironPtr @system.
Use a ReadWriteMutex to protect reading and writing to environment.
Add scope to getImpl callback parameter.

Warning 1: This (currently) removes nothrow @nogc from remove.
Warning 2: I am not that experienced with locks (particularly on Windows), so bear that in mind, I may have done something wrong. Or there may be a better solution, please let me know.

[dlang-bot]

Thanks for your pull request and interest in making D better, @ntrel! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the coverage diff by visiting the details link of the codecov check)
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

⚠️⚠️⚠️ Warnings ⚠️⚠️⚠️

• In preparation for migrating from Bugzilla to GitHub Issues, the issue reference syntax has changed. Please add the word "Bugzilla" to issue references. For example, Fix Bugzilla Issue 12345 or Fix Bugzilla 12345.(Reminder: the edit needs to be done in the Git commit message, not the GitHub pull request.)

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + phobos#10611"

[0xEAB]

Improper rebase, my bad, will redo.

[0xEAB]

Properly rebased now.
See https://github.com/0xEAB/phobos/tree/ntrel/env-mutex-backup for a backup of the original branch.

[0xEAB]

I think it would be better if the final version had braces and indentation around the synchronized blocks.
But for now, having them shoe-horned in keeps the diff straightforward.

[0xEAB]

For changelog reasons, the commit message would ideally be prefixed with Fix #10580 – if it fixes said issue completely.

[schveiguy]

I'm not sure we can remove those attributes, this might break code.

Why does this now require GC and throwing? Can we catch any exceptions?

[schveiguy]

Also, the comment for this function should be altered, as now we are supporting multithreaded changes via our own lock.

[schveiguy]

This at least needs a test.

[schveiguy]

Thinking about this, I'm not sure I like this approach. It depends on implementation details of C, and avoiding calling C functions that might alter the environment.

Consider this, if you alter the environment in another thread not through this interface, you get the same problem. Yet the interface is explicitly designed to be able to see such changes.

My approach that I recommend is to store a shadow copy of the environment, which updates the environment on set, and is properly synchronized (a rwlock is a decent choice for this). You sync the environment from C with the first call, and then on subsequent calls, you use the internal copy for everything, pushing changes back to C.

Unfortunately, I think this is the only way to keep the functions @safe, only the initial read from libc will need to be trusted.

The nogc thing is a harder problem, but still possible to deal with by using C malloc instead of an AA for storage.

What do you think?

[pbackus]

Fundamentally, modifying the environment is not thread safe, and there is no amount of synchronization or caching we can do in D to fix that.

Rust ran into the same issue: rust-lang/rust#27970

[LightBender]

@schveiguy @pbackus @ntrel

This looks stalled and there appears to be no solution even in glibc. Do we close this PR?

[0xEAB]

there is no amount of synchronization or caching we can do in D to fix that.

If that’s the case, we’re doomed either way and can close this with no outcome for now.
We should prolly keep this in mind for Phobos v3 where we would make environment operations @system

[schveiguy]

There isn't a solution that makes this @safe, @pbackus is right, setting the C environment pointer via setenv is thread-unsafe, and no amount of wrapping can fix this. We can make reads @safe though, by shadowing the environment.

I think that's probably the only path forward.