You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@thrift.apache.org by "jnordstrom-iar (via GitHub)" <gi...@apache.org> on 2026/09/17 13:03:07 UTC
[PR] Keep the TPipe implementation alive while a read or write uses it [thrift]
jnordstrom-iar opened a new pull request, #3878:
URL: https://github.com/apache/thrift/pull/3878
<!-- Explain the changes in the pull request below: -->
This another (old) patch we've had inhouse. The test is completely written by AI.
On Windows there is no supported way to stop a pipe server while clients are still connected: `TPipeServer` does not implement `interruptChildren()`, and `TPipe` has neither an `interrupt()` nor a read timeout, so `TServer::stop()` cannot wake a worker blocked in `read()`. See: https://issues.apache.org/jira/browse/THRIFT-3590
A workaround we've had is to track connected clients through `TServerEventHandler` and then `close()` their transports from the thread that stops the server. That is not safe on master. `close()` destroys the implementation object a concurrent `read()` is still using. This change guards the implementation pointer with a critical section and hands each caller its own reference, so the implementation stays alive for the duration of the call. `close()` now also detaches the implementation before releasing it and then cancels its outstanding I/O, so a reader waiting for data returns instead of waiting on an implementation that is already gone.
`stress_pipe_close_during_use` dies inside the transport in 19 of 20 runs without the change and passes 20 of 20 with it (MSVC, RelWithDebInfo).
<!-- We recommend you review the checklist/tips before submitting a pull request. -->
- [ ] ~~Did you create an [Apache Jira](https://issues.apache.org/jira/projects/THRIFT/issues/) ticket? ([Request account here](https://selfserve.apache.org/jira-account.html), not required for trivial changes)~~
- [ ] ~~If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?~~
- [X] Did you squash your changes to a single commit? (not required, but preferred)
- [X] ~~Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"~~
- [ ] ~~If your change does not involve any code, include `[skip ci]` anywhere in the commit message to free up build resources.~~
<!--
The Contributing Guide at:
https://github.com/apache/thrift/blob/master/CONTRIBUTING.md
has more details and tips for committing properly.
-->
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6329: Keep the TPipe implementation alive while a read or write uses it [thrift]
Posted by "jnordstrom-iar (via GitHub)" <gi...@apache.org>.
jnordstrom-iar commented on PR #3878:
URL: https://github.com/apache/thrift/pull/3878#issuecomment-5797609341
@Jens-G I'm not that familiar with low-level pipe programming in Windows and not the Thrift repo in general (and neither is anyone at the company). I've dispatched a few coding agents on trying to solve this, but the solutions are quite different and the diffs can be large, so it's difficult predict the implications of those changes.
What do you think the best approach for introducing these fixes? For example, (a) a minimal patch scoped tightly to our use case, with lower risk but narrower coverage, or (b) a more general fix that's more "correct" but touches more code and is harder to verify?
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6329: Keep the TPipe implementation alive while a read or write uses it [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3878:
URL: https://github.com/apache/thrift/pull/3878#issuecomment-5801761595
Ok, I'll have a look but not today.
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6329: Keep the TPipe implementation alive while a read or write uses it [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3878:
URL: https://github.com/apache/thrift/pull/3878#issuecomment-5847805044
@jnordstrom-iar Thanks for the patch and for filing THRIFT-6329. On your (a)/(b) question: your approach stays. The reference-counted implementation under a lock is the right fix for the lifetime problem. What was missing is waking up the I/O that is still waiting, so I completed that part and pushed it to your branch as one squashed commit, with you as the author, rebased onto current master.
What changed on top of your commit:
- **`cancel()` works for both named-pipe implementations.** It sets a manual-reset event that is never reset. Every overlapped wait (`pseudo_sync_read`, `pseudo_sync_write`, and the waitable read) waits for both its I/O event and that cancel event. If the cancel event fires, the wait calls `CancelIoEx()` for its own `OVERLAPPED` and then waits for that operation to finish before it returns. It is built this way so that a cancel that arrives before the I/O is issued is not lost, and so that data that has already arrived is still returned.
- **A cancelled read or write throws `TTransportException::INTERRUPTED` and logs nothing**, so `TConnectedClient` ends quietly. With the first version of this PR:
- a server-side read ended with `UNKNOWN` and a logged `GetOverlappedResult` failure;
- a client-side read and every write kept waiting.
- **`setPipeHandle()` cancels the implementation it replaces**, as `close()` does.
- **Anonymous pipes use synchronous I/O and are not interrupted.** The comment on `close()` in the header says so.
- **Tests:** three new cases with a real peer that never sends: `TPipeServer` plus `TPipe::open()`.
- The cases are a client-side read, a server-side read, and a server-side write into a full pipe.
- Each case checks that the call is still blocked after 200 ms and that it ends with `INTERRUPTED` within 10 s of `close()`.
- All waits have a time limit, so a regression fails the test instead of hanging CI.
- Your stress test stays and now uses the same helper.
Verified on Windows (GitHub-hosted runner, MSVC, Release). Each case ran 10 times, each run in its own process:
| | client read | server read | server write | stress test |
|---|---|---|---|---|
| master + new tests | fails 10/10: `UNKNOWN` ("GetOverlappedResult failed") | fails 10/10: `UNKNOWN` | fails 10/10: not woken up within 10 s | crashes 10/10 (ctest: SegFault) |
| first version of this PR + new tests | fails 10/10: not woken up within 10 s | fails 10/10: `UNKNOWN` | fails 10/10: not woken up within 10 s | passes 10/10 |
| this PR now | passes 10/10 | passes 10/10 | passes 10/10 | passes 10/10 |
The whole `TInterruptTest` (socket, SSL and pipe cases) passes with the change. The three runs are [master + new tests](https://github.com/Jens-G/thrift/actions/runs/36246399228), [first version + new tests](https://github.com/Jens-G/thrift/actions/runs/36246399205) and [this change](https://github.com/Jens-G/thrift/actions/runs/36246399324). They ran on a scaffolding branch that builds only `TInterruptTest`; the three changed files are the same as in the commit pushed here.
Your underlying use case, stopping a pipe server while clients are connected, is THRIFT-3590. With the cancel event in place, `TPipeServer::interruptChildren()` becomes a small follow-up: the server hands one shared interrupt event to every pipe it accepts. That would make the event-handler workaround unnecessary, and it would be a separate PR.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep the TPipe implementation alive while a read or write uses it [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3878:
URL: https://github.com/apache/thrift/pull/3878#issuecomment-5749066071
@jnordstrom-iar Could you please adress the findings? Thank you!
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep the TPipe implementation alive while a read or write uses it [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6329: Keep the TPipe implementation alive while a read or write uses it [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G merged PR #3878:
URL: https://github.com/apache/thrift/pull/3878
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6329: Keep the TPipe implementation alive while a read or write uses it [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3878:
URL: https://github.com/apache/thrift/pull/3878#issuecomment-5854223621
@emmenlau Can you pls have a look too? I'd like a second opinion here.
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] THRIFT-6329: Keep the TPipe implementation alive while a read or write uses it [thrift]
Posted by "Jens-G (via GitHub)" <gi...@apache.org>.
Jens-G commented on PR #3878:
URL: https://github.com/apache/thrift/pull/3878#issuecomment-5849775029
### Code review
Re-reviewed at `a60a35c8d`. No blocking issues found. Checked for bugs and CLAUDE.md compliance.
Both issues from the last round are fixed: `cancel()` now wakes a waiting read or write on both named-pipe implementations, and the PR has its THRIFT-6329 ticket.
One suggestion, below the bar for an issue but verified:
- For anonymous pipes, `close()` no longer closes the pipe handles while another thread is inside `read()` or `write()`. `TAnonPipeImpl` does synchronous I/O and has no `cancel()` of its own, and the call in flight now holds the last reference, so both `TAutoHandle`s stay open until that call returns. A peer that waits for our end-of-file before it closes its own end therefore waits as long as our reader does, where `close()` used to close both handles at once.
I checked this with a smoke probe, a MinGW build of `TPipe.cpp` run under Wine 10, not on real Windows. It uses `TPipe(readHandle, writeHandle)`, one thread blocked in `read()`, and a peer thread reading the other pipe. With master's `TPipe.cpp` the peer sees end-of-file about 100 ms after `close()`. With this PR it is still waiting after 3 s, and gets it only once it closes its own write end, which also ends our `read()`.
The old behaviour came from destroying the implementation under the running call, which is the lifetime problem this PR removes, so bringing it back may not be wanted. The comment on `close()` could say that for anonymous pipes the handles stay open until a pending call returns.
https://github.com/apache/thrift/blob/a60a35c8de782f4fc7ab9f3cfc32e13dfc51cefd/lib/cpp/src/thrift/transport/TPipe.h#L74-L79
https://github.com/apache/thrift/blob/a60a35c8de782f4fc7ab9f3cfc32e13dfc51cefd/lib/cpp/src/thrift/transport/TPipe.cpp#L364-L372
🤖 Generated with [Claude Code](https://claude.ai/code)
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Keep the TPipe implementation alive while a read or write uses it [thrift]
Posted by "jnordstrom-iar (via GitHub)" <gi...@apache.org>.
jnordstrom-iar commented on PR #3878:
URL: https://github.com/apache/thrift/pull/3878#issuecomment-5756507806
> @jnordstrom-iar Could you please adress the findings? Thank you!
Yes, I'll look into them!
--
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: notifications-unsubscribe@thrift.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org