Jens-G commented on PR #3878: URL: https://github.com/apache/thrift/pull/3878#issuecomment-5721063897
### Code review Found 2 issues: 1. `close()` no longer unblocks a reader on the ordinary client path, because `cancel()` is a no-op for `TNamedPipeImpl` and `TAnonPipeImpl` (bug due to `TPipeImpl::cancel()` being overridden only by `TWaitableNamedPipeImpl`) `close()` hands its reference to a concurrent reader and then calls `oldImpl->cancel()`: https://github.com/apache/thrift/blob/4c6ceee0a401f49bec21bf07fae11db8a2032d7b/lib/cpp/src/thrift/transport/TPipe.cpp#L314-L323 `TWaitableNamedPipeImpl` is the only class that implements `cancel()`: https://github.com/apache/thrift/blob/4c6ceee0a401f49bec21bf07fae11db8a2032d7b/lib/cpp/src/thrift/transport/TPipe.cpp#L117-L119 `TNamedPipeImpl` and `TAnonPipeImpl` inherit the base no-op: https://github.com/apache/thrift/blob/4c6ceee0a401f49bec21bf07fae11db8a2032d7b/lib/cpp/src/thrift/transport/TPipe.cpp#L46-L48 and `TPipe::open()` (like the non-anonymous branch of `setPipeHandle()` at L392) installs `TNamedPipeImpl`: https://github.com/apache/thrift/blob/4c6ceee0a401f49bec21bf07fae11db8a2032d7b/lib/cpp/src/thrift/transport/TPipe.cpp#L311-L313 On that path the reader now holds the last reference, so the `TAutoHandle` member is not destroyed and `CloseHandle()` is not called, while `cancel()` does nothing. A thread blocked in `pseudo_sync_read()`'s `GetOverlappedResult(pipe, &tempOverlap, &bytes, TRUE)` therefore stays blocked. Previously `close()` was `impl_.reset()`, which released the implementation and closed the handle, completing the pending operation and returning control to the reader. So for `TPipe::open()` clients and for anonymous pipes, `close()` no longer ends that wait, which is the opposite of what the comment on those lines states. Only the `TPipe(TAutoHandle&)` and `TPipe(HANDLE)` constructors build `TWaitableNamedPipeImpl`, so the server-accepted named-pipe path the PR targets does get the intended wake-up. `stress_pipe_close_during_use` cannot catch this: it goes through `setPipeHandle()`, so every iteration uses `TNamedPipeImpl`, and its reads fail fast with `ERROR_PIPE_LISTENING` rather than pending. Either implement `cancel()` for the other two implementations or narrow the comment to the waitable case. 2. A non-trivial change with no JIRA ticket and no `THRIFT-NNNN:` prefix on the title or commit subject (AGENTS.md says "All significant changes need a JIRA ticket." and, for the JIRA row, "Required for all non-trivial PRs") The GitHub-issues row is scoped to "Typos, trivial compiler warnings, etc."; this change adds a `TCriticalSection` plus two guarded accessors to `TPipe.h`, adds a `cancel()` virtual to the `TPipeImpl` hierarchy, and changes the concurrency semantics of `close()`/`read()`/`write()`. THRIFT-3590 is cited as background, but it is a separate open ticket that this PR does not resolve. The `Client: cpp` line and the AI attribution trailer are both present and correct. https://github.com/apache/thrift/blob/4c6ceee0a401f49bec21bf07fae11db8a2032d7b/AGENTS.md#L27-L34 🤖 Generated with [Claude Code](https://claude.ai/code) <sub>- If this code review was useful, please react with 👍. Otherwise, react with 👎.</sub> -- 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]
