Jens-G opened a new pull request, #3914: URL: https://github.com/apache/thrift/pull/3914
> **Stacked on [#3913](https://github.com/apache/thrift/pull/3913) (THRIFT-6063)**, which adds the CI job and the runner these tests need. Review that first; this branch contains it. `TTransport>>readAll:` asks `read:` for the bytes it still needs and appends whatever comes back, until it has enough: ```smalltalk [str size < anInteger] whileTrue: [str nextPutAll: (self read: anInteger - str size)] ``` A `read:` that answers an empty string appends nothing, and asking the same source again for the same bytes cannot answer differently — so the loop has no way to end. **Not reachable as the library stands.** Nothing in `thrift.st` answers an empty string from `read:`; `TSocket>>read:` signals instead. This is a contract that any new or third-party transport can break, and breaking it *hangs* the caller rather than raising. `c_glib` had the same shape in `thrift_transport_real_read_all()` and was given an explicit progress check. `readAll:` now signals `TTransportError` when `read:` answers nothing — which is exactly what `TSocket>>read:` already does for the same condition, so the behaviour is consistent within the binding. ## Why the tests look the way they do The failure being tested for is a **hang, not an exception**. A test that simply calls `readAll:` on a stalled transport would not report a failure on unfixed code — it would sit there until the CI job hit its own timeout, which is a much worse way to find out. So every case runs inside `valueWithin: 10 seconds onTimeout: [self fail: ...]`. New `TTransportReadAllTest` suite, two stub transports — one answering nothing at all, one answering at most *n* bytes per call: | test | pins down | |---|---| | `testStalledTransportSignalsRatherThanSpinning` | raises instead of looping | | `testStalledTransportStopsAskingAfterTheFirstEmptyRead` | asked **once**, not repeatedly | | `testPartialThenStalledSignals` | bytes arrived, then the source dried up → error, not a short result | | `testChunkedTransportStillAssembles` | **the case the loop exists for** still works (3 reads, correct bytes) | | `testExactlyOneReadWhenTheTransportAnswersEverything` | no redundant second read | | `testZeroBytesNeedsNoRead` | `readAll: 0` touches the transport not at all | ## Two-state verification Against the **unmodified** library: `RUN=6 PASS=3 ERROR=3` — exactly the three stalled cases fail, and they fail *within the timeout* rather than hanging. With the fix: `RUN=6 PASS=6`. Full run on this branch — 23 tests across three suites: ``` TProtocolStringSizeLimitTest 11 of 11 passed TTransportReadAllTest 6 of 6 passed TProtocolRecursionDepthTest 6 of 6 passed ``` ## Note [THRIFT-6300](https://issues.apache.org/jira/browse/THRIFT-6300) routes `readInt:`, `readRawInt:` and `readString` through `readAll:`, which turns this unreachable loop into a reachable one. That is why this lands first. JIRA: [THRIFT-6258](https://issues.apache.org/jira/browse/THRIFT-6258) 🤖 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
