[
https://issues.apache.org/jira/browse/GROOVY-12459?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18129214#comment-18129214
]
ASF GitHub Bot commented on GROOVY-12459:
-----------------------------------------
daniellansun commented on PR #2987:
URL: https://github.com/apache/groovy/pull/2987#issuecomment-6294131593
@paulk-asert Thank you for the careful read, and for checking the behaviour
locally. The three changes you asked for are in, and the documentation points
are covered as well.
## `close(Throwable)` returns `boolean`
`AsyncChannel.close(Throwable)` now returns `boolean`, the same way
`close()` does. `true` means this call closed the channel. The first close
wins, so a later call returns `false` and leaves the retained failure
unchanged. `null` is a normal close.
The default method calls `close()` and returns that result. It does not
retain the cause, so an implementation that does not override the method still
ends iteration instead of throwing. `DefaultAsyncChannel` retains the cause and
returns the same boolean from `finishClose`.
`BroadcastChannel.close(Throwable)` returns the same boolean.
`BroadcastChannel.close()` stays `void`, because that method shipped in 6.0.0.
## `merge` closes on the first failure
When one input fails, the merged channel closes with that failure. It does
not stay open until the other input ends. Values a relay has already taken, and
values already buffered on the other input, are forwarded first. The other
input is left open and is not read further. A producer that has not yet handed
a value over is not released. When both inputs fail, the first failure retained
on the output wins, and a later normal close does not replace it.
## Javadoc and the user guide
The Javadoc now describes the current contract. The note about a channel
compiled against 6.0.0 is gone; that history belongs in the commit message. The
guide says "a custom `AsyncChannel` that does not override `close(Throwable)`".
The guide also says:
- a `receive` / `send` loop must read `ChannelClosedException.getCause()`,
or the failure is dropped
- plain `for`, `each`, `collect`, and `toList` use the same iterator and
throw the failure
- a Java enhanced `for` calls `Iterator.hasNext()`, which can throw a
checked exception that `Iterator` does not declare
- a normal close and a failure close are different in `select`: a buffered
value is still delivered, and the failure is the result only when nothing
buffered remains. Which one is met first follows registration order (list
order, `fair()`, or `random()`)
The failure section no longer names C#, JavaScript, or Swift.
## Follow-ups
A `produce { }` helper would remove the try / close / catch boilerplate,
with `close(Throwable)` still the primitive underneath. I agree that is the
groovier next step, and I have left it as a follow-up so this change stays the
contract fix.
The `BroadcastChannel` subscribe / close race is a separate fix inside the
same commit. I will name it in the commit message when this lands. Suggested
sentence: "Also close the race between `BroadcastChannel.subscribe` and
`close`."
## Sonar
`volatile Throwable closeCause` is the reference publication Sonar flags as
`java:S3077`, which is what failed the reliability rating on new code. The
field is marked the same way as the other immutable holders in the tree. The
`Throwable` is not mutated after it is published.
> 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)
