windows: stabilise inherit_handles

[nia-e]

Relevant to #161158; closes #146407 (by stabilising it). See my comment on the tracking issue.

r? libs

[purplesyringa]

I'd like to bikeshed the name inherit_handles a little. (Obviously this is not a blocker.) If we eventually add a more idiomatic, feature-complete, or platform-independent API for controlling inheritance, we'd probably want to give it a clearer/shorter name to motivate using it. Taking this into account, would it be reasonable to rename this into something like inherit_handles_indiscriminately? Personally I like it better because it hints that there is no safe allow-list, and because it leaves the name inherit_handles or equivalent to something that takes an allow-list into account (even if it's going to be on an attribute builder rather than Command itself).

[ChrisDenton]

If we eventually add a more idiomatic, feature-complete, or platform-independent API for controlling inheritance

The current Unix equivalent is just .fd(...) (see #144989) so I'd assume the Windows equivalent would be .handle(...).

I would prefer we keep the name inherit_handles because that's the name of the CreateProcess argument after converting it to use rust's naming convention (bInheritHandles but without the type and using underscores). We should only promise that it causes the flag to be passed to CreateProcess, no more or less.

I do however think we need to be sure that in stabilising this we aren't closing off any potential other APIs for handle inheritance. Or if we are then we're happy to do so. Should this override other APIs that may want to set it? Or should it be given the lowest priority? Or should it be a "last one wins" situation.

[nia-e]

I think this shouldn't exclude any other proposals and I see it as a convenience method that just so happens to be stabilisable before the more complex one. In my mind the semantics are that there is one list of handles to inherit, and this method has the effect of clearing/filling that list (so, last one wins). it would be shorthand for .set_inherited_handles(&[]) or .set_inherited_handles(&[ /* all of them */ ]), using ad-hoc method names

[ChrisDenton]

.set_inherited_handles(&[ /* all of them */ ])

I would note that /* all of them */ is not really possible because there's no good way to get all inheritable handles. EDIT: there are bad ways, but not something we want to encourage.

[PaulDance]

Thanks for starting this process (heh).

I do think there is indeed something to figure out about the potential interactions it could have with such a future API. Personally, I find APIs that successfully abstract away from the lower-level stuff to be nice when they plug in well to the higher-level stuff (for example Rust semantics in general), but ones that explicitly don't do that in certain cases to be nicer if they regard relatively low-level stuff that is not very well controlled anyway. I think the current case could be akin to the latter.

I guess a good way to decide that would be to determine the following: is there really a need to couple the two APIs together somehow? That is to say: what would happen if the two were to be implemented completely independently of one another in terms of low-level stuff? Would it lead to easily-avoidable errors or misuses that a better API could help remove entirely or would it just work, i.e. unused parameters are just ignored by the Windows API? That might require experimentation to be sure. Obviously, ignored parameters could be considered as a blocking issue by itself because they would represent a form of request or intent from the user that is not respected in the end, so could just lead to confusion.

Listing the cases:

• .inherit_handles(true) alone cannot easily be translated to some set_inherited_handles call, as noted above, so would just have its own meaning of "any and all applicable handles currently available in the parent process";
• .inherit_handles(false) alone should probably be equivalent to .set_inherited_handles(&[]) alone in most cases, but in some it is known not be in practice, as noted for example in Ability to stop child process from Inheriting Handles libs-team#264 (comment), so should probably be made to be equivalent if that is desirable;
• .inherit_handles(true) and .set_inherited_handles(&[]) should be equivalent to .inherit_handles(false) in practice, but might need to be made so for the same reasons as above;
• .inherit_handles(true) and .set_inherited_handles(&[/* some */]) would have no conflict and would mean "inherit these among the inheritable ones", i.e. "restrict the list to only these instead of all";
• .inherit_handles(false) and .set_inherited_handles(&[]) would have no conflict and should be the same as using one or the other alone;
• .inherit_handles(false) and .set_inherited_handles(&[/* some */]) would be the most conflicting case; making it equivalent to .inherit_handles(false) could help stay consistent with the above because "restrict the list to these" while "the list" has been emptied would lead to an empty list still;

So in summary, both .inherit_handles(false) and .set_inherited_handles(&[]) would have precedence over anything else they are combined with, except with a repetition of the same method of course.

"Last one wins" could be a strategy to resolve the conflicting cases instead of this, but I'm not sure it would be the greatest if assigning particular semantics to each method is desired like done above. It would be much simpler though, so could be better than the above that could be deemed too convoluted and therefore hard to memorize. It would also be consistent with same-method repetitions.

In any case though, I don't see how choosing any of these options could shape the current API differently?