SteNicholas commented on code in PR #3801: URL: https://github.com/apache/celeborn/pull/3801#discussion_r3780892801
########## cpp/celeborn/network/MessageDispatcher.cpp: ########## @@ -178,7 +209,21 @@ folly::Future<std::unique_ptr<Message>> MessageDispatcher::operator()( this->pipeline_->write(std::move(toSendMsg)); Review Comment: **[P1] Please observe and handle the write future here.** Wangle's `AsyncSocketHandler::write` returns an exceptional future immediately when the socket is no longer good, and its asynchronous `writeErr` also completes that future exceptionally. Those failures do not necessarily set this dispatcher's `closed_` before the trailing check, so discarding the future can leave the registered request promise pending until its timeout—the hang this change is intended to avoid. Please attach an error continuation that removes and fails the corresponding registry entry retriably, and cover both RPC/push and fetch with a mock handler that returns a failed write future. Reference: https://github.com/facebook/wangle/blob/v2024.07.01.00/wangle/channel/AsyncSocketHandler.h#L95-L113 ########## cpp/celeborn/network/MessageDispatcher.cpp: ########## @@ -163,6 +186,14 @@ folly::Future<std::unique_ptr<Message>> MessageDispatcher::operator()( } } + // Fast path: the connection is already closed. Fail with a retriable error + // rather than asserting, so the caller's retry/failover logic can recover. + if (closed_.load()) { + return folly::makeFuture<std::unique_ptr<Message>>( + makeConnectionClosedException(fmt::format( Review Comment: **[P2] This retriable classification is discarded at the next public API layer.** `TransportClient::sendRpcRequestSync` catches the exception and rethrows via `CELEBORN_FAIL` (`isRetriable=false`), while the push and fetch `thenError` handlers convert it to `std::runtime_error`. Consequently callers cannot observe the `isRetriable=true` value asserted by these dispatcher-only tests. Please preserve the `CelebornException` classification through `TransportClient` and add API-level tests for the sync, push, and fetch paths. -- 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]
