Add ability to customize STARTUPINFO structure in Command API

[federico-terzi]

Proposal

Problem statement

I need a way to customize the STARTUPINFO structure when spawning a new Command on Windows

Motivating examples or use cases

I'm working on a desktop application which needs to often spawn sub-processes as part of its regular operation. The Command API works well in general, but there's a problem: by default, on Windows every sub-process causes the parent window to display a "spinner" (aka feedback) cursor until the sub-process displays a window. This becomes problematic when the sub-processes are worker processes that will never display a window, as it makes the parent app appear unresponsive due to the spinning cursor.

In Windows land, this is easily solvable by passing the STARTF_FORCEOFFFEEDBACK flag to the STARTUPINFO struct, which is then passed to the CreateProcessW (see SO question)

Unfortunately, while the Command API exposes a creation_flags parameter, this only influences the CreateProcessW flags, not the STARTUPINFO flags.

Solution sketch

Ideally, the Command API should expose some methods to configure the startupinfo_flags individually. After a discussion, we propose to expose the following methods:

// Controls STARTF_RUNFULLSCREEN
fn startupinfo_fullscreen(&mut self, enabled: bool)

// Controls STARTF_UNTRUSTEDSOURCE
fn startupinfo_untrusted_source(&mut self, enabled: bool)

// Controls STARTF_FORCEONFEEDBACK and STARTF_FORCEOFFFEEDBACK
fn startupinfo_force_feedback(&mut self, enabled: Option<bool>)

The STARTUPINFO struct accepts also other flags, but their use is less straightforward as they have side-effects and relationships with other fields, so they can't be changed in isolation (see follow-up comment from @joshtriplett for more details).

Alternatives

For this specific problem, I'm using a workaround to post a dummy message on the sub-process (see SO question). This unfortunately still causes the loading cursor to appear for a fraction of a second, so it's not a solution.

Links and related work

• https://internals.rust-lang.org/t/add-ability-to-customize-startupinfo-structure-in-std-command-api/22569
• https://stackoverflow.com/questions/4942664/avoid-hourglass-after-calling-createprocess
• https://learn.microsoft.com/en-us/windows/win32/api/processthreadsapi/ns-processthreadsapi-startupinfoa

I believe this would be a great addition to the standard library, as it's similar to the existing creation_flags argument

[joshtriplett]

cc @ChrisDenton for Windows review.

Doing a quick review of some of the flags in dwFlags, to figure out if we need to filter any of them out for safety, or add anything else to go with them:

• We should filter out any unknown flags, because those flags could in the future be assigned to something unsafe. For instance, a new flag could be "this new field of the structure contains a pointer to ...". So, we should only allow known-safe flags.
• STARTF_FORCEONFEEDBACK and STARTF_FORCEOFFFEEDBACK are fine.
• STARTF_PREVENTPINNING says that it requires STARTF_TITLEISAPPID, so the issues with STARTF_TITLEISAPPID below apply. If we can support STARTF_TITLEISAPPID, then allowing this flag should be fine, as it sounds like it'll just error if you use one without the other.
• STARTF_RUNFULLSCREEN is fine.
• STARTF_TITLEISAPPID affects the interpretation of lpTitle, so I don't think we can let this function set it. It'd need to be set by a separate functions that sets the AppID, if supported, and then we set this flag if and only if we set the AppID. We'd then need to only allow STARTF_PREVENTPINNING when the AppID is set.
• STARTF_TITLEISLINKNAME likewise affects the interpretation of lpTitle, and is mutually exclusive with STARTF_TITLEISAPPID. So, if we support this at all, we'd need to handle it with a separate function.
• STARTF_UNTRUSTEDSOURCE is fine.
• All of the STARTF_USE* flags affect the interpretation of other fields of the structure, so they shouldn't be allowed unless the corresponding field of the structure is also set, which means they probably want separate setter functions.

Given all the above, it sounds like the only flags that a startupinfo_flags method should accept are STARTF_FORCEONFEEDBACK, STARTF_FORCEOFFFEEDBACK, STARTF_PREVENTPINNING (if we add the separate method for setting the AppID), STARTF_RUNFULLSCREEN, and STARTF_UNTRUSTEDSOURCE. All other flags should be filtered out.

For the other flags:

• We could add a startupinfo_appid method taking an Option<String> (or something conveniently convertible into a wide string, whatever approach @ChrisDenton would suggest for that).
• We could add a startupinfo_linkname method, if there's any call for it, but it sounds like that's not commonly used, so perhaps we should skip it until someone asks for it.
• For all the USE flags, we could add methods like startupinfo_position (taking an Option of a pair of values of the right type), startupinfo_size (likewise), startupinfo_countchars (likewise), and similar. We'd set the flags if we set the fields, and clear the flags if clearing the fields.

All of those are optional; we should filter out those flags, but we don't have to add methods for them right away.

Lastly, because there are so few flags that can be set this way and don't need separate setter methods, I wonder if it'd be more user-friendly to just directly provide setter methods for those specific flags, rather than having a single method that takes a combination of flags. For instance, we could have startupinfo_fullscreen (taking bool), startupinfo_untrusted_source (taking bool), startupinfo_force_feedback (taking Option<bool>, since the two flags seem mutually exclusive), and so on.

@ChrisDenton, @federico-terzi, thoughts?

[ChrisDenton]

My only issue would be, as you say, safely exposing an option which later turns out to be unsafe. This happened with OpenOptionsExt::custom_flags and I'd prefer not to repeat that mistake.

So I'm happy with either having an unsafe way to set STARTUPINFO or a safe way to set individual flags (along with any data if required). If we do the latter than I'd agree that we don't need to design all the functions right away. Adding them on an as-needed basis is fine. Starting with STARTF_FORCEONFEEDBACK/STARTF_FORCEOFFFEEDBACK sounds reasonable.

[joshtriplett]

Between those two options, I would favor adding safe methods for the known safe flags, and we can always add the unsafe mechanism later if we can't expose some piece of functionality in a safe way.

[federico-terzi]

thoughts?

Thank you for the great analysis! I think individual flags (eg. startupinfo_force_feedback) as you proposed could be a great solution

[joshtriplett]

Could you please update the ACP with those APIs?

[joshtriplett]

We discussed this in today's @rust-lang/libs-api meeting and agreed that this should be individual flag methods.

If you get a chance to update the ACP, we can then review and approve that ACP in a future meeting.

[federico-terzi]

Sorry for the radio silence! That's great, I'll make sure to update the ACP soon

[federico-terzi]

Sorry for the delay, I've updated the ACP with the methods as you suggested

[Amanieu]

Thanks! We discussed this again in the @rust-lang/libs-api meeting and are happy to accept this.

Feel free to open a tracking issue and open a PR to rust-lang/rust to add it as an unstable feature.

[federico-terzi]

Thank you, will do it soon!

[federico-terzi]

@Amanieu @joshtriplett Thank you for all the input! I've opened a tracking issue and PR:

• Tracking issue: Tracking Issue for windows_process_extensions_startupinfo rust#141010
• PR: Implement methods to set STARTUPINFO flags for Command API on Windows rust#141013

[Amanieu]

Discussion should continue in the tracking issue.