paulk-asert commented on PR #3100:
URL: https://github.com/apache/groovy/pull/3100#issuecomment-6535207495
Approving - what is proposed is a real step forward but I'll note these
minor comments in case we spot issues down the track:
> Things the reviewers didn't mention (not blocking):
>
> 1. A slow machine could make forAwaitOverPublisher hang instead of fail.
waitUntil gives up after 2 s by default and throws inside the async block.
> - Nobody awaits that task, so the exception is silently lost. The
broadcast is never closed, and the main thread's for await blocks forever. The
repo has no default JUnit timeout to stop it.
> - The old 50 ms sleep could only miss values and fail an assertion.
> - The sibling forAwaitOverPublisherThrowsARetainedFailure already has
the same shape, so it's consistent. Closing the broadcast in a try { … }
finally { broadcast.close() } around the async body would turn the hang into a
failure. That would be a reasonable follow-up, or a suggestion on the PR.
> 2. The AsyncAwaitSpecTest change appears in the user guide. The
fastest_server tag is included in core-async-await.adoc, so readers will now
see Thread.sleep(5000).
> - It reads fine for a slow server.
> - The losing task isn't cancelled, so a pool thread sleeps for 5 s
after the test passes. That's harmless.
>
> Minor points:
> - The fast/slow ChannelSelectTest now checks which branch index is
reported rather than a race. That's fine, since the race was never
deterministic anyway.
> - If the deadline in the timer test fires early, the producer stays
blocked on take(), which leaks one thread. The test still fails cleanly.
>
> Lost coverage: ActorTest.testScheduleAtFixedRateFiresRepeatedly.
> - The old test asked for at least 3 ticks in 300 ms on a 40 ms period.
That caught a schedule that repeats but far too slowly.
> - The new test only asks for 3 ticks within 5 s. A period wrongly taken as
400 ms would fail the old test and pass the new one, though 40 seconds would
still fail.
> - What's still covered is "it repeats and doesn't stop after one tick",
which is what the test name and the new comment claim. Any fixed time window
will fail on a loaded machine, so I think this trade is the right one. If you
still want the rate checked, a looser check that can't flake would do it, for
example asserting the time between ticks is well under a second.
--
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]
