FrankChen021 commented on code in PR #19006:
URL: https://github.com/apache/druid/pull/19006#discussion_r3141586452
##########
processing/src/main/java/org/apache/druid/frame/channel/BlockingQueueFrameChannel.java:
##########
@@ -261,7 +293,12 @@ public ListenableFuture<?> readabilityFuture()
public void close()
{
synchronized (lock) {
- queue.clear();
+ if (readerClosed) {
+ // close() should not be called twice.
+ throw DruidException.defensive("Closed, cannot call close() again");
+ }
+
+ readerClosed = true;
Review Comment:
P1 Readable close can leave writers permanently blocked
Readable.close() now only sets readerClosed and notifies the current writer
future, but it leaves the queue contents in place and
Writable.writabilityFuture() does not consider readerClosed. If the queue is
full when the reader is closed, the notified writer can re-check writability,
see the queue still full, create a new readyForWritingFuture, and wait forever
because no reader will drain the queue or notify again. This can hang upstream
processors or result producers when a consumer cancels or closes early.
Consider clearing the queue as before, or making readerClosed close/fail the
writable side so future waiters resume deterministically.
##########
processing/src/main/java/org/apache/druid/frame/processor/ReturnOrAwait.java:
##########
@@ -106,7 +106,7 @@ public static <T> ReturnOrAwait<T> awaitAll(final int count)
/**
* Wait for all of the provided futures.
*/
- public static <T> ReturnOrAwait<T> awaitAllFutures(final
Collection<ListenableFuture<?>> futures)
+ public static <T> ReturnOrAwait<T> awaitAllFutures(final
List<ListenableFuture<?>> futures)
Review Comment:
P2 Public awaitAllFutures signature breaks callers
Changing awaitAllFutures from Collection<ListenableFuture<?>> to
List<ListenableFuture<?>> changes the public static method descriptor. Existing
compiled extensions or downstream modules that call the Collection overload can
fail with NoSuchMethodError, and source callers with another Collection type no
longer compile. Keep the Collection overload, or retain the original signature
and defensively copy to a List internally if list semantics are required.
--
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]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
