[
https://issues.apache.org/jira/browse/GROOVY-12326?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18110474#comment-18110474
]
ASF GitHub Bot commented on GROOVY-12326:
-----------------------------------------
codecov-commenter commented on PR #2853:
URL: https://github.com/apache/groovy/pull/2853#issuecomment-5502676516
##
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/2853?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
Report
:x: Patch coverage is `94.44444%` with `1 line` in your changes missing
coverage. Please review.
:white_check_mark: Project coverage is 70.7950%. Comparing base
([`bc9db9f`](https://app.codecov.io/gh/apache/groovy/commit/bc9db9f75615c3d4ebe10391ceb142d69e077644?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
to head
([`8c84340`](https://app.codecov.io/gh/apache/groovy/commit/8c84340a7fb5934198bef713f789d885d06f378c?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/2853?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Patch % | Lines |
|---|---|---|
|
[src/main/java/groovy/concurrent/ChannelSelect.java](https://app.codecov.io/gh/apache/groovy/pull/2853?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fgroovy%2Fconcurrent%2FChannelSelect.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9ncm9vdnkvY29uY3VycmVudC9DaGFubmVsU2VsZWN0LmphdmE=)
| 94.4444% | [0 Missing and 1 partial :warning:
](https://app.codecov.io/gh/apache/groovy/pull/2853?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/2853?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #2853 +/- ##
==================================================
- Coverage 70.7952% 70.7950% -0.0002%
- Complexity 36973 36979 +6
==================================================
Files 1576 1576
Lines 134786 134792 +6
Branches 24952 24954 +2
==================================================
+ Hits 95422 95426 +4
- Misses 30760 30765 +5
+ Partials 8604 8601 -3
```
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/2853?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Coverage Δ | |
|---|---|---|
|
[src/main/java/groovy/concurrent/ChannelSelect.java](https://app.codecov.io/gh/apache/groovy/pull/2853?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Fgroovy%2Fconcurrent%2FChannelSelect.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9ncm9vdnkvY29uY3VycmVudC9DaGFubmVsU2VsZWN0LmphdmE=)
| `89.5652% <94.4444%> (+0.5744%)` | :arrow_up: |
... and [10 files with indirect coverage
changes](https://app.codecov.io/gh/apache/groovy/pull/2853/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test
Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests,
report on failures, and find test suite problems.
- :package: [JS Bundle
Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save
yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
> ChannelSelect: guard an offer directly with receive(c).when(cond)
> -----------------------------------------------------------------
>
> Key: GROOVY-12326
> URL: https://issues.apache.org/jira/browse/GROOVY-12326
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Priority: Major
>
> h2. Summary
> GROOVY-12324 added a precondition mask to {{ChannelSelect}}, and it works.
> Building a real guarded ALT
> against it surfaced a second, separable gap: the mask binds each flag to an
> offer *positionally and
> anonymously*.
> {code:groovy}
> def alt = ChannelSelect.from(put, get) // written here
> ...
> def r = await alt.select(counter < cap, counter > 0) // ...guarded here, by
> position only
> {code}
> Nothing at the selection site says which channel {{counter < cap}} guards.
> The reader counts arguments and
> matches them against a {{from(...)}} written elsewhere -- possibly far away,
> since the instance is normally
> hoisted so that {{fair()}} keeps its rotation state (GROOVY-12320). That is
> the same class of fragility the
> mask was introduced to remove; GROOVY-12324 stopped the *index* from moving,
> and left the *flag*'s
> association to a branch just as implicit.
> This proposes attaching the guard to the offer itself, which is what occam,
> Ada, Erlang and Kotlin all do.
> h2. Proposal
> {code:java}
> // on ChannelSelect.Offer
> public Offer when(java.util.function.BooleanSupplier enabled)
> {code}
> {code:groovy}
> def alt = ChannelSelect.offers(receive(put).when { counter < cap },
> receive(get).when { counter > 0 }).fair()
> while (true) {
> def r = await alt.select()
> ...
> }
> {code}
> h3. Why a supplier and not a plain boolean
> This is the part that only shows up when you build with it. A guard must be
> re-evaluated on every
> selection, because the condition it tests changes every iteration -- that is
> the whole point of a bounded
> buffer. An {{Offer}} is documented as "Immutable and freely reusable across
> selects", and a select instance
> is normally built ONCE and hoisted out of the loop so that {{fair()}} can
> keep its rotation state.
> So {{when(boolean)}} would be wrong in two different ways:
> * built once and hoisted, it freezes the guard at construction and every
> later selection uses a stale flag;
> * rebuilt each iteration to avoid that, it discards the {{lastWinner}}
> rotation -- reintroducing exactly the
> bug GROOVY-12320 was filed to fix, and which the checker already refuses as
> "fair() on a fresh
> ChannelSelect instance each iteration keeps no rotation state (priority in
> effect)".
> A {{BooleanSupplier}} evaluated at each {{select()}} call is the only form
> that keeps a held instance and a
> per-iteration guard at the same time. In Groovy it reads as a closure, and a
> closure over a mutable local
> sees the updates, so the bounded-buffer idiom above works as written.
> h3. Interaction with select(boolean...)
> An offer's {{when}} and a flag passed to {{select(boolean...)}} should be
> *conjoined*: the offer is enabled
> when both hold. That is the only reading that does not silently ignore one of
> the two. (Rejecting the
> combination outright is also defensible; what should not happen is one
> quietly winning.)
> Everything else is inherited unchanged: indices are preserved, and if every
> offer ends up disabled the
> returned awaitable already fails with {{IllegalStateException}}.
> h2. Open question -- worth settling before the API ships
> Nothing carrying GROOVY-12324 has been released yet, so the shape is still
> free. That makes this the moment
> to ask a question that cannot be asked afterwards: *do we want both forms, or
> should {{when}} be the only
> one?*
> The argument for keeping both is that they serve different entry points
> rather than duplicating each other:
> * {{select(boolean...)}} works directly with {{from(channels...)}}, which is
> what most code writes, and is
> the direct port of JCSP's {{priSelect(boolean\[\])}} and GPars'
> {{select(List<Boolean>)}} -- useful to
> anyone migrating;
> * {{when}} needs {{offers(receive(c)...)}}, which is more verbose for the
> common all-receives case, but
> binds guard to branch syntactically.
> If both are kept, the javadoc should lead with {{when}} as the idiomatic form
> and present the mask as the
> positional/compatibility one. If only one survives, it should be {{when}}.
> h2. Prior art
> Attaching the guard to the branch is the majority design; the positional mask
> is the minority one, and it
> exists mainly where the result is a bare index.
> || System || Guard is written... || Bound to its branch by ||
> | occam / occam-pi | {{bool & c ? x}} | syntax |
> | Ada | {{when Cond =>}} on a select alternative; entry barriers | syntax |
> | Erlang / Elixir | {{Pattern when Guard ->}} | syntax |
> | Kotlin coroutines | conditional clause registration in {{select \{ \}}} |
> the clause itself |
> | Go | {{nil}} channel in the case expression | the case |
> | JCSP | {{priSelect(boolean\[\] preCondition)}} | position |
> | GPars | {{select(List<Boolean> mask)}} | position |
> | Groovy 6 today | {{select(boolean... enabled)}} | position |
> h2. Implementation sketch
> Small, and contained to {{Offer}} plus one line in the registration loop.
> * {{Offer}} gains a {{BooleanSupplier enabled}} field, null meaning "always".
> {{when(...)}} returns a new
> {{Offer}} with the same channel/value/send and the given supplier,
> preserving immutability and
> reusability.
> * {{registrationOrder}} already skips offers the mask disables; extend its
> test from
> {{enabled == null || enabled\[i\]}} to also consult
> {{offers.get(i).enabled}}, evaluating the supplier
> once per {{select()}} call. The existing empty-order path then produces the
> {{IllegalStateException("every offer of the select is disabled")}} with no
> further change.
> * No change to {{Result}}, the claim protocol, {{withdraw}} or {{resend}}.
> * {{from(...)}} is unaffected: it builds plain receive offers with no guard.
> h2. Provenance
> Found by building a static verifier's model of GROOVY-12324 against Jon
> Kerridge's UCaPE c05 {{Queue}}
> (the bounded circular buffer that masks both of its guards) -- the same tool
> and the same corpus behind
> GROOVY-12320, GROOVY-12323 and GROOVY-12324. The mask itself is not at fault
> and needs no change; this is
> about where the guard is written.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
