[Breaking Change Request] Throw StateError in meaningless getters on detached processes

[sortie]

Intended change

The Process class will now throw a StateError if the process is detached (ProcessStartMode.detached and ProcessStartMode.detachedWithStdio) upon accessing the exitCode getter. It now also throws when not connected to the child process's stdio (ProcessStartMode.detached and ProcessStartMode.inheritStdio) upon accessing the stdin, stdout, and stderr getters. Previously these getters would all return null.

Rationale

Child processes started with ProcessStartMode.detached or ProcessStartMode.detachedWithStdio are detached and the current process is not notified when the child exits. The Process.exitCode getter is meaningless in this case and currently returns null.

Likewise child processes started with ProcessStartMode.detached or ProcessStartMode.inheritStdio do not have pipes connected and the Process.stdin, Process.stdout, and Process.stderr getters are meaningless and currently return null.

As part of the effort to make Dart null-safe, we would like to make these getters throw a StateError in these cases instead. This change would allow us to mark the Process.exitCode, Process.stdin, Process.stdout, and Process.stderr getters as non-null. The alternative is to mark the getters as nullable, however all meaningful uses of the getters will always be non-null, yet all those uses would be required to check for null. We would like avoid those null checks that will never fail by making the getters non-nullable because we throw instead.

Expected impact

package:bazel_worker prior to 0.1.23+1 was accessing the exitCode getter on a detached process, but did nothing with it, which now crashes with a StateError ("Bad state: Process is detached") as of this breaking change. Upgrade to bazel_worker-0.1.23+1 or later to fix the issue.

Otherwise it did not make sense to use these getters when they could be null and such uses should be very rare.

Steps for mitigation

If package:bazel_worker is failing with a StateError ("Bad state: Process is detached"), then upgrade to bazel_worker-0.1.23+1 or later to fix the problem.

If any code is checking whether a Process is detached by checking if these getters return null, it will need to be changed to check if a StateError is thrown.

Implementation

A proposed implementation of this breaking change: https://dart-review.googlesource.com/c/sdk/+/134329

cc @franklinyow @mit-mit @lrhn @athomas

[sortie]

This breaking change is currently landed in the NNBD-migrated SDK, but not the regular SDK. It would be prudent to land this breaking change in the regular SDK prior to unforking the SDK to avoid blocking problems at that time. If this breaking change cannot be made, the NNBD-migrated SDK will need to be changed to make the getters nullable.

I will be unavailable next week. Depending on the NNBD unforking schedule, someone else may need to take over landing this breaking change if it's approved.

[jakemac53]

One thing to note here is you can't easily (as far as I can tell) check whether a process instance is detached or not - so you essentially have to fall back on a try/catch for any api which accepts a Process.

If that sort of api is relatively uncommon though it might not matter.

[franklinyow]

cc @Hixie @matanlurey @dgrove for review and approval.

[matanlurey]

Seems fine

[Hixie]

[vsmenon]

lgtm for dart

[franklinyow]

Approved
cc: @lrhn @athomas @a-siva

[sortie]

Thanks for the approval. I'll go ahead and land this breaking change tomorrow.

[sortie]

I've now landed this change. See the mitigation instructions above if there is any breakage.

One thing to note here is you can't easily (as far as I can tell) check whether a process instance is detached or not - so you essentially have to fall back on a try/catch for any api which accepts a Process.

You can catch the StateError to find out, or check for null as well if you want to support older SDKs. However, your code should already know whether the process is detached or not. You cannot do anything useful with a Process object unless you know things about it, such as what stdin/stdout/stderr means and whether they are attached at all (and whether other code is already providing/consuming them), and so on. Your code will need to keep track of those assumptions itself and whether a process is detached or not is part of those assumptions.

[jakemac53]

However, your code should already know whether the process is detached or not.

This is exactly the problem - you can't always know. In the case of bazel_worker it accepts a function that spawns a process. It has no control over how that process is created or whether it is detached or not.

Any api which has to deal with a process that it didn't spawn can't know in what mode it was spawned 🤷‍♂️ .

[sortie]

This is probably not the right place to discuss that particular issue. I do understand how this change broke bazel_worker and that it's not useful to debate in hindsight how the code should've been written. We're making this breaking change because it'll improve the overall experience for almost all code using the Process class and what bazel_worker was doing is a rare exception. I didn't see any way to avoid this breakage without making the API worse in the long term, sorry.

In general, you can't do anything meaningful given an arbitrary Process object. There's always some assumptions about the process that are baked into your code. Even if you receive an arbitrary function that creates a process, you have assumptions about what kinds of processes it may create. For instance, bazel_worker collects stdout of the process, so you don't allow ProcessStartMode.detached but do allow ProcessStartMode.detachedWithStdio. Likewise you assume no other code is consuming the stdout stream.