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: **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 -- 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]
