paulk-asert commented on PR #2987:
URL: https://github.com/apache/groovy/pull/2987#issuecomment-6276887941
AI read:
> PR #2987 fixes a real problem and the approach is sound, but it needs a
few changes before merging. I'd ask for three changes (listed below) and also
suggest a producer helper that closes the channel automatically. It's from
daniellansun, one commit `570c9124bc`, +1019/−107.
>
> ## What it does
> Until now, a producer that threw before calling `close()` left `for await`
waiting forever, and the exception was lost. The PR adds `close(Throwable)` and
`getCloseCause()` to `AsyncChannel`, both as default methods.
`DefaultAsyncChannel` keeps the failure and attaches it as the cause of
`ChannelClosedException`. Its iterator rethrows the failure itself, which is
what makes `for await` throw. `map`, `filter`, `merge`, `split` and `tap` now
pass a failure through, `ChannelSelect` fails with it, and `BroadcastChannel`
gets `close(Throwable)` and reports it to subscribers as `onError`.
>
> **Testing:** CI ran 33/33 checks with 122,256 tests passing, but the
SonarCloud gate failed on "Reliability Rating on New Code". The Sonar lookup
was blocked here, so I don't know which issue it is. Locally on JDK 17, the
PR's own tests plus some probe tests I wrote all pass.
>
> ## What's good
> - **The model has precedent.** Kotlin's `Channel.close(cause)` works the
same way: values already sent still arrive, then the failure.
> - **Old custom channels can't hang.** The default `close(Throwable)` just
calls `close()`, so a custom implementation built against 6.0.0 still ends the
loop.
> - **Composition is cleaner.** One shared `pump`/`offer` helper replaces
five copies of the receive loop. A callback that throws inside `map` or
`filter` is no longer lost on an executor thread.
> - **`ChannelSelect` is careful.** A failure can't override a transfer that
has already committed.
> - **It also fixes an older race.** `BroadcastChannel.subscribe` used to
race with `close`; it now takes the same lock.
>
> ## Changes I'd request
> 1. **Make `close(Throwable)` return `boolean`.** `close()` returns
`boolean` but the new method returns `void` (probe confirmed), while Kotlin's
`close(cause)` returns `Boolean`. This is easy to change now and impossible
after release.
> 2. **`merge` holds the failure back.** When one input fails and the other
is still open, the merged channel stays open with no cause. The failure only
shows up once the other input closes, so a long-running second input hides it
indefinitely. The merged output should close with the failure straight away.
> 3. **The Javadoc describes history.** For example: "so a channel compiled
against 6.0.0 does not hang". It should describe current behaviour, and the
history belongs in the commit message.
>
> ## Design points to raise
> - **Hand-written receive loops still drop the failure.** `receive` and
`send` throw `ChannelClosedException` with the failure as its cause, while
iteration throws the failure itself. Many people end a manual loop with `catch
(ChannelClosedException)`, which is exactly what the library's own operators
did before this PR. Such loops end quietly and lose the failure (probe
confirmed). Kotlin avoids this by throwing the cause directly from `receive`.
If this design stays, the docs should tell people writing manual loops to check
`getCause()`.
> - **More than `for await` is affected.** `for await` is just the channel's
iterator, so plain `for`, `each`, `collect` and `toList` on a failed channel
now throw too (probe: `each` threw `IOException` after `[1, 2]`). Java for-each
loops get an undeclared checked exception from `hasNext()`, which Java code
can't catch by its specific type. This deserves a sentence in the docs.
> - **The producer side is still mostly boilerplate.** Every producer has to
write try / close / catch / `close(failure)` itself, which is roughly the trap
the issue describes. I'd suggest a helper like Kotlin's `produce { }`: it runs
the closure asynchronously and closes the channel normally or with the failure.
`close(Throwable)` would remain the underlying method. This is probably the
"groovier" answer Daniel is looking for, and it could be a follow-up.
> - **Select treats the two kinds of close differently.** A channel closed
normally is skipped until every channel is closed, but a channel closed with a
failure fails the whole select. Which one wins against a ready value depends
only on the order the channels are listed (50/50 when I alternated the order).
That's consistent and matches Kotlin; it's just worth knowing.
>
> ## Minor
> - **Docs style.** The user guide names C#, JavaScript and Swift, which is
unusual for the Groovy docs. "A channel compiled against the previous release"
means a custom `AsyncChannel` implementation and should say so.
> - **Bundled fix.** The `BroadcastChannel` subscribe/close race fix is a
separate bug fix inside this PR. That's fine, but the commit message should
mention it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
