[
https://issues.apache.org/jira/browse/GROOVY-12459?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18128898#comment-18128898
]
ASF GitHub Bot commented on GROOVY-12459:
-----------------------------------------
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.
> Failed AsyncChannel producer never closes the channel so `for await` hangs
> and the exception is dropped
> -------------------------------------------------------------------------------------------------------
>
> Key: GROOVY-12459
> URL: https://issues.apache.org/jira/browse/GROOVY-12459
> Project: Groovy
> Issue Type: Bug
> Reporter: Daniel Sun
> Priority: Major
>
> h2. Context
> {{AsyncChannel}} is the channel used with {{async}} and {{for await}}. A
> producer sends values and then calls {{close()}} so the consumer can finish.
> That shape shipped with the channel API. Native async/await is GROOVY-9381.
> Later channel work includes GROOVY-12320 ({{ChannelSelect}}) and GROOVY-12343
> ({{AsyncChannel.after}}).
> The proposed change keeps that close and lets it carry the producer failure.
> The code that fails, and the code that reads, each write their own {{try}} /
> {{catch}}.
> h2. Problem
> {code:groovy}
> def channel = AsyncChannel.create(2)
> async {
> for (int i = 1; i <= 3; i++) {
> someCall()
> channel.send(i * i)
> }
> channel.close()
> }
> for await (value in channel) {
> println value
> }
> {code}
> If {{someCall()}} throws, both of these happen:
> * {{channel.close()}} never runs, so {{for await}} waits forever.
> * The exception stays on the {{Awaitable}} returned by {{async}}. The example
> discards that handle, so the consuming code never sees the failure.
> A {{try}} / {{finally}} inside the task, followed by {{await}} on the handle,
> fixes both symptoms, but the consumer is waiting on the channel, not on the
> handle. {{defer}} runs on the way out of the task, including when the task
> throws. A {{defer}} block that calls {{channel.close()}} closes the channel
> with no cause, so {{for await}} ends normally and the exception stays on the
> handle.
> h2. Change
> The producer catches the failure and closes with it. Values already sent are
> still delivered. {{receive}} and {{send}} then fail with
> {{ChannelClosedException}}, and that failure is the cause. {{for await}}
> throws the failure itself after those values, including a checked exception
> or an {{Error}}. The reader writes the {{try}} / {{catch}}, as with C#
> {{await foreach}}, JavaScript {{for await}}, and Swift {{for try await}}.
> {code:groovy}
> def channel = AsyncChannel.create(2)
> async {
> try {
> for (int i = 1; i <= 3; i++) {
> someCall()
> await channel.send(i * i)
> }
> channel.close()
> } catch (Exception failure) {
> channel.close(failure)
> }
> }
> def seen = []
> try {
> for await (value in channel) {
> seen << value
> }
> } catch (Exception failure) {
> // failure is the exception thrown by someCall()
> }
> {code}
> {{close(failure)}} returns nothing. {{null}} is a normal close. The first
> close wins.
> ||What happened||What the consumer sees||
> |{{close()}}, or {{close(null)}}|{{ChannelClosedException}} with no cause.
> {{for await}} ends. {{ChannelSelect}} still treats this as a normal close.|
> |{{close(failure)}} on a channel that retains it|{{receive}} and {{send}}
> fail with {{ChannelClosedException}} whose cause is {{failure}}. {{for
> await}} and {{ChannelSelect}} throw {{failure}} after the values already
> sent.|
> |Every selected channel closed normally|{{ChannelSelect}} fails with a
> {{ChannelClosedException}} whose message is "all channels are closed".|
> |A transfer already committed when another channel closes|That transfer stays
> the result. A withdrawn branch, and {{CancellationException}}, are not
> closes.|
> {{filter}}, {{map}}, {{merge}}, {{split}}, and {{tap}} carry a source
> failure, or a failure thrown by the callback, through to their output.
> Closing one side of a {{split}} stops the split and closes the other side
> normally. A failure close of one side is not copied onto the other.
> {{BroadcastChannel.close(failure)}} closes every subscriber the same way.
> {{for await}} on a subscriber throws that failure, and the publisher view
> reports it with {{onError}}.
> {{getCloseCause()}} reports the retained failure, or {{null}} when the
> channel is open, was closed normally, or does not retain a cause.
> h2. Compatibility
> {{close(Throwable)}} and {{getCloseCause()}} are default methods, added after
> the 6.0.0 channel API. A class compiled against that API does not need to be
> recompiled and does not throw {{AbstractMethodError}}.
> * The default {{close(Throwable)}} calls {{close()}} and does not retain the
> cause. {{for await}} on that channel ends. It does not throw the failure.
> * The channel from {{AsyncChannel.create}} does retain the cause. {{close()}}
> and {{close(Throwable)}} share one close, so neither calls the other. The
> cause is stored before the closed flag, under the same lock.
> * An implementation of {{close()}} must also override {{close(Throwable)}} if
> its {{close()}} calls that method. The default calls {{close()}} again.
> * A resource that lives only inside an {{async}} task is unchanged. Release
> it with {{defer}}, and observe the task with {{await}} or with
> {{scope.async}} inside {{AsyncScope.withScope}}.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
