Skip to content

Commit 0a08ad0

Browse files
robertvokacrobertvokac
committed
fix(tasks): validate ParallelOptions::MaxDegreeOfParallelism in its setter (#2388)
#1969's other half, filed and closed the same day. MaxDegreeOfParallelism is now private behind getMaxDegreeOfParallelismProperty() / setMaxDegreeOfParallelismProperty(), with .NET's two guards transcribed from Parallel.cs:85-90. Landed under SA-8. #1966 had already landed the same rule, but at the entry of every Parallel method, because a public data member has nowhere to put a check -- and its doc-comment recorded that as a forced choice awaiting this approval. The difference is observable, not cosmetic: an invalid degree used to be STORED and survive until a loop ran, so a caller that assigned and never looped got no diagnostic at all, and one that read the value back read a number .NET can never hold. No accepted value changed meaning and no rejected value became accepted. Only the moment of rejection moved. The parameter name stays "MaxDegreeOfParallelism", and that is confirmed rather than assumed: .NET writes nameof(MaxDegreeOfParallelism) here and nameof(value) in BoundedChannelOptions. The reference is inconsistent between its two option types; both are transcribed as they are, and harmonising this onto #1969's "value" would be inventing a reference. Also confirmed: .NET's other two ParallelOptions members are trivial get/set, so only this one was in scope. One #1966 test moved rather than being left to pass for the wrong reason. It asserted that the degree error beats the empty-body error, because in .NET the setter runs before Parallel.For is called. With the guard in the setter there is no ordering left to assert -- the invalid degree cannot reach Parallel::For at all, and leaving the old test would have had the throw come from the assignment, outside the EXPECT_THROW, which is the #2359 trap. Five mutations caught, M4 by the new pin and four pre-existing #1966 cases. One equivalence recorded rather than counted: removing the now-unreachable use-site guard changes nothing, because the field is private, its only mutator validates, and the private member makes ParallelOptions a non-aggregate. It is kept as defence in depth and the site says so. Fixture set 39/207 -> 40/211. Downstream measured separately from #1969's: 0 sites in cna, 0 in mobile-eggbert. Gate: 17,434 run, 17,434 passed, 0 failed, 0 skipped across 38 executables (+4 on 17,430; SharpRuntimeTests_Threading_Tasks 222 -> 226; no other executable moved). Module graph unchanged at 41/93.
1 parent caee29a commit 0a08ad0

7 files changed

Lines changed: 341 additions & 46 deletions

File tree

‎CLAUDE.md‎

Lines changed: 1 addition & 1 deletion
Large diffs are not rendered by default.

‎docs/Migration-ParallelOptionsMaxDegreeSetter.md‎

Lines changed: 115 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,115 @@
1+
<!-- SPDX-License-Identifier: MIT -->
2+
<!-- Copyright (c) Robert Vokac and contributors -->
3+
4+
# Migration — `ParallelOptions::MaxDegreeOfParallelism` is validated in its setter (ticket #2388)
5+
6+
*2026-08-19.* `System::Threading::Tasks::ParallelOptions::MaxDegreeOfParallelism` was a bare
7+
public mutable data member. It is now private, behind
8+
`getMaxDegreeOfParallelismProperty()` / `setMaxDegreeOfParallelismProperty()`, and an invalid
9+
degree is rejected **at assignment** — where .NET rejects it.
10+
11+
Landed under `docs/StandingApprovals.md` **SA-8**, with SA-2's five conditions discharged.
12+
Split out of #1969 rather than bundled with it, because the approval question there was about
13+
`BoundedChannelOptions` and the consumer measurement is per-type.
14+
15+
---
16+
17+
## 1. What changed
18+
19+
| | Was | Is |
20+
|---|---|---|
21+
| read | `opts.MaxDegreeOfParallelism` | `opts.getMaxDegreeOfParallelismProperty()` |
22+
| write | `opts.MaxDegreeOfParallelism = n` | `opts.setMaxDegreeOfParallelismProperty(n)` |
23+
| `0` or `< -1` | **stored**, rejected later when a loop ran | rejected **at assignment** |
24+
| exception & parameter name | `ArgumentOutOfRangeException("MaxDegreeOfParallelism")` | unchanged |
25+
| default | `-1` | unchanged |
26+
| valid values | `-1` and every `>= 1` | unchanged |
27+
| `ParallelOptions` aggregate-ness | aggregate | **no longer an aggregate** |
28+
29+
**No accepted value changed meaning, and no rejected value became accepted.** Only the moment of
30+
rejection moved.
31+
32+
## 2. Why the move matters
33+
34+
Ticket #1966 landed this validation already — but at the entry of every `Parallel` method,
35+
because a public data member has nowhere to put a check. Its doc-comment recorded that as a
36+
forced choice awaiting an approval, and named #1969 as the gating ticket.
37+
38+
The difference is observable, not cosmetic:
39+
40+
```cpp
41+
ParallelOptions opts;
42+
opts.MaxDegreeOfParallelism = 0; // used to succeed
43+
auto d = opts.MaxDegreeOfParallelism; // reads 0 -- a value .NET can never hold
44+
// ...and if no loop is ever run, no diagnostic is ever produced.
45+
```
46+
47+
.NET's setter (`Parallel.cs:85-90`) is two guards:
48+
49+
```csharp
50+
ArgumentOutOfRangeException.ThrowIfZero(value, nameof(MaxDegreeOfParallelism));
51+
ArgumentOutOfRangeException.ThrowIfLessThan(value, -1, nameof(MaxDegreeOfParallelism));
52+
```
53+
54+
so `_maxDegreeOfParallelism` can never hold an invalid number in the first place.
55+
56+
## 3. The parameter name is `"MaxDegreeOfParallelism"`, and that is not a slip
57+
58+
#1969 landed the sibling change on `BoundedChannelOptions::FullMode` with the parameter name
59+
`"value"`. This one keeps `"MaxDegreeOfParallelism"`. **Both match their own reference**:
60+
61+
| Type | .NET writes |
62+
|---|---|
63+
| `BoundedChannelOptions.FullMode` | `nameof(value)` — `ChannelOptions.cs:96` |
64+
| `ParallelOptions.MaxDegreeOfParallelism` | `nameof(MaxDegreeOfParallelism)` — `Parallel.cs:87-88` |
65+
66+
The reference is inconsistent between its two option types. Both are transcribed as they are;
67+
harmonising them would be inventing a reference where SA-5 says to derive one. `#1966` had
68+
already got this right, and this ticket deliberately does not "fix" it.
69+
70+
## 4. One test moved rather than being left to pass for the wrong reason
71+
72+
#1966 asserted that an invalid degree beats an empty body — the degree error must be reported
73+
first, because in .NET the setter runs before `Parallel.For` is called at all.
74+
75+
**With the guard in the setter there is no longer an ordering to assert.** The invalid degree
76+
cannot reach `Parallel::For`; it is refused at the assignment. Leaving the old test would have
77+
made it pass for the wrong reason — the throw would come from the assignment, *outside* the
78+
`EXPECT_THROW` — which is exactly the trap #2359 hit. It is replaced by
79+
`Fix2388_AnInvalidDegreeCannotReachParallelForAtAll`, which asserts the stronger property and
80+
then shows the options object is still usable, because a rejected assignment leaves the previous
81+
value in place.
82+
83+
## 5. The use-site guard is kept, and its mutation is an equivalence
84+
85+
`requireValidMaxDegreeOfParallelism` still runs at every `Parallel` entry point, and it is now
86+
**unreachable through the public surface**: the field is private, its only mutator validates, and
87+
the private member makes `ParallelOptions` a non-aggregate, so brace initialisation cannot reach
88+
it either.
89+
90+
It is kept — one comparison per loop — as the only thing that would catch a future constructor or
91+
friend that sets the field directly. Its mutation is therefore recorded as an **equivalence**
92+
rather than counted as a caught mutation, at the site and here.
93+
94+
## 6. Evidence
95+
96+
Mutations on the setter, **all caught**:
97+
98+
| Mutation | Caught by |
99+
|---|---|
100+
| M1 — the setter stores without validating | `Fix2388_ZeroAndBelowMinusOneAreRejectedAtAssignment` and the `ParallelDegreeBoundaryTests` family |
101+
| M2 — the `value == 0` arm dropped | the same, on the `0` row specifically |
102+
| M3 — the bound becomes `value < 0`, rejecting the valid `-1` | `Fix2388_MinusOneAndEveryPositiveAreAccepted` |
103+
| M4 — parameter name becomes `"value"` (i.e. #1969's) | `Fix2388_TheParameterNameIsThePropertyNotValue` |
104+
| M5 — rejection half-applies before throwing | `Fix2388_ZeroAndBelowMinusOneAreRejectedAtAssignment` |
105+
| E1 — remove the now-unreachable use-site guard | **not caught — an equivalence**, §5 |
106+
107+
Negative consumer fixture: `test/consumer/threading_tasks_maxdegree_private_negative.cpp`, four
108+
sites, all rejected. Fixture set grows to **40 fixtures / 211 sites**. Site 3 is the spelling that
109+
used to compile *and stick*; site 4 is the aggregate-ness a consumer loses silently.
110+
111+
## 7. Downstream, measured
112+
113+
Per SA-2 condition 5, and measured separately from #1969's rather than assumed to match it:
114+
`MaxDegreeOfParallelism` appears in **zero** places in `cna` and **zero** in `mobile-eggbert`.
115+
Neither repository was modified, and no downstream ticket is needed.

‎modules/threading-tasks/include/System/Threading/Tasks/Parallel.hpp‎

Lines changed: 56 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -29,42 +29,56 @@ namespace System::Threading::Tasks {
2929
/** @brief Stores options that configure the operation of methods on the Parallel class. */
3030
struct ParallelOptions {
3131
/**
32-
* Maximum number of concurrent iterations; -1 (the default) means unlimited.
33-
*
34-
* @note **Valid values are -1 and every value greater than or equal to 1.** 0 and every
35-
* value less than -1 are rejected with `System::ArgumentOutOfRangeException`
36-
* (ticket #1966, SR-AUD-232).
37-
*
38-
* @note **Where the rejection happens differs from .NET, deliberately.** .NET validates
39-
* in the `ParallelOptions.MaxDegreeOfParallelism` *setter*, so an invalid value can never
40-
* be stored. This is a public mutable data member with nowhere to put a check, so the
41-
* value is validated at the entry of every `Parallel` method that reads it, before any
42-
* iteration is dispatched. The exception type and parameter name are .NET's; only the
43-
* point of detection moves. Converting this field to a
44-
* `getMaxDegreeOfParallelismProperty()`/`setMaxDegreeOfParallelismProperty()` pair would
45-
* be a public source break for every consumer that writes `opts.MaxDegreeOfParallelism =
46-
* …`, and is deliberately **not** done here.
47-
*
48-
* **That deferral is now the only thing left, and it has an approval.** The identical
49-
* shape problem in `BoundedChannelOptions::FullMode` was approval-gated as ticket #1969,
50-
* and #1969 **landed** on 2026-08-19 under `docs/StandingApprovals.md` SA-8, whose first
51-
* bullet authorises exactly this conversion. Moving the guard here to a setter is ticket
52-
* **#2388**.
53-
*
54-
* One detail #2388 must not "harmonise": this port's parameter name
55-
* `"MaxDegreeOfParallelism"` is already correct — .NET writes
56-
* `ArgumentOutOfRangeException.ThrowIfZero(value, nameof(MaxDegreeOfParallelism))`
57-
* (`Parallel.cs:87-88`), where `BoundedChannelOptions` writes `nameof(value)`. The
58-
* reference is inconsistent between its two option types, and both are transcribed as
59-
* they are.
32+
* @return The maximum number of concurrent iterations; -1 means unlimited.
6033
*
6134
* @note -1 keeps its documented "unlimited" meaning; this runtime realises it as
6235
* `std::thread::hardware_concurrency()`, which is what .NET's own default does. That is a
6336
* *worker* count chosen at runtime and is explicitly **not** a CLAUDE.md
6437
* build-resource-policy violation, which concerns build-job counts
6538
* (`docs/ThreadingTasksChannelsReviewPlan.md` §3.1 item 3).
6639
*/
67-
intcs MaxDegreeOfParallelism = -1;
40+
[[nodiscard]] intcs getMaxDegreeOfParallelismProperty() const noexcept {
41+
return maxDegreeOfParallelism_;
42+
}
43+
44+
/**
45+
* @brief Sets the maximum number of concurrent iterations.
46+
* @param value -1 ("unlimited") or any value greater than or equal to 1.
47+
* @throws System::ArgumentOutOfRangeException if @p value is 0 or less than -1
48+
* (parameter name `MaxDegreeOfParallelism`).
49+
*
50+
* Transcribed from .NET's setter (`Parallel.cs:85-90`), which is two guards in this
51+
* order:
52+
* @code
53+
* ArgumentOutOfRangeException.ThrowIfZero(value, nameof(MaxDegreeOfParallelism));
54+
* ArgumentOutOfRangeException.ThrowIfLessThan(value, -1, nameof(MaxDegreeOfParallelism));
55+
* @endcode
56+
*
57+
* @note **The parameter name is the property, not "value"** — and that is .NET's own
58+
* inconsistency rather than a slip here. `BoundedChannelOptions` writes `nameof(value)`
59+
* in the same situation (`ChannelOptions.cs:96`). Both are transcribed as they are;
60+
* harmonising them would be inventing a reference (#1969, #2388).
61+
*/
62+
void setMaxDegreeOfParallelismProperty(intcs value) {
63+
if (value == 0 || value < -1)
64+
throw System::ArgumentOutOfRangeException("MaxDegreeOfParallelism");
65+
maxDegreeOfParallelism_ = value;
66+
}
67+
68+
private:
69+
// PRIVATE, matching .NET's `private int _maxDegreeOfParallelism` (Parallel.cs:35).
70+
//
71+
// Ticket #1966 could not put the guard where .NET puts it, because this was a public
72+
// mutable data member with nowhere to put a check; it validated at the entry of every
73+
// Parallel method instead, and its doc-comment recorded that as a forced choice awaiting
74+
// the approval #1969 was gated on. SA-8 granted it, #1969 landed the identical change for
75+
// BoundedChannelOptions::FullMode on 2026-08-19, and this is #2388.
76+
//
77+
// The observable difference is real, not cosmetic: an invalid degree used to be STORED
78+
// and survive until a loop ran, so a caller that assigned and never looped got no
79+
// diagnostic at all, and one that assigned and read the value back read a number .NET
80+
// would never have let it hold.
81+
intcs maxDegreeOfParallelism_ = -1;
6882
};
6983

7084
/**
@@ -211,6 +225,16 @@ namespace System::Threading::Tasks {
211225
// called and therefore before .NET's own `body` null check; putting the checks the other
212226
// way round would report the body error for a call .NET answers with the degree error.
213227
// Same shape as docs/ThreadingNamespaceReviewPlan.md 17.3's constraint on #1954.
228+
// #2388 MOVED THE REAL GUARD INTO ParallelOptions::setMaxDegreeOfParallelismProperty,
229+
// where .NET has it, so this one is now DEFENCE IN DEPTH and is UNREACHABLE through the
230+
// public surface: the field is private, its only mutator validates, and the private
231+
// member makes ParallelOptions a non-aggregate, so brace initialisation cannot reach it
232+
// either. Its mutation is therefore an EQUIVALENCE and is recorded as one rather than
233+
// counted as a caught mutation.
234+
//
235+
// It is kept rather than deleted because it costs one comparison per loop and is the
236+
// only thing that would catch a future constructor or friend that sets the field without
237+
// going through the setter.
214238
static void requireValidMaxDegreeOfParallelism(intcs maxDegree) {
215239
if (maxDegree == 0 || maxDegree < -1)
216240
throw System::ArgumentOutOfRangeException("MaxDegreeOfParallelism");
@@ -234,21 +258,21 @@ namespace System::Threading::Tasks {
234258
/**
235259
* Executes a for loop in parallel, respecting MaxDegreeOfParallelism in @p opts.
236260
*
237-
* @throws System::ArgumentOutOfRangeException if `opts.MaxDegreeOfParallelism` is 0 or
261+
* @throws System::ArgumentOutOfRangeException if the degree is 0 or
238262
* less than -1 (parameter name `MaxDegreeOfParallelism`) — checked first, because .NET
239263
* rejects those values in the options setter, which runs before this call. No iteration
240264
* is dispatched when it throws.
241265
* @throws System::ArgumentNullException if @p body is empty (parameter name `body`).
242266
*/
243267
static ParallelLoopResult For(intcs fromInclusive, intcs toExclusive, const ParallelOptions& opts,
244268
std::function<void(intcs)> body) {
245-
requireValidMaxDegreeOfParallelism(opts.MaxDegreeOfParallelism);
269+
requireValidMaxDegreeOfParallelism(opts.getMaxDegreeOfParallelismProperty());
246270
requireNonEmptyBody(body);
247271
#if defined(__EMSCRIPTEN__) && !defined(__EMSCRIPTEN_PTHREADS__)
248272
(void)fromInclusive; (void)toExclusive; (void)opts; (void)body;
249273
throw System::PlatformNotSupportedException("Parallel::For requires pthreads (not available in Emscripten single-threaded build)");
250274
#else
251-
const intcs maxDeg = resolveMaxDegreeOfParallelism(opts.MaxDegreeOfParallelism);
275+
const intcs maxDeg = resolveMaxDegreeOfParallelism(opts.getMaxDegreeOfParallelismProperty());
252276

253277
std::vector<std::future<void>> futures;
254278
std::vector<std::exception_ptr> exceptions;

0 commit comments

Comments
 (0)