Skip to content

THRIFT-6144: Report EOF when Ruby transport reads make no progress - #3708

Open
kpumuk wants to merge 1 commit into
apache:masterfrom
kpumuk:rb-read-all-progress
Open

THRIFT-6144: Report EOF when Ruby transport reads make no progress#3708
kpumuk wants to merge 1 commit into
apache:masterfrom
kpumuk:rb-read-all-progress

Conversation

@kpumuk

@kpumuk kpumuk commented Aug 12, 2026

Copy link
Copy Markdown
Member

Ruby transports may report end of input by returning nil or an empty string. BaseTransport#read_all currently leaks a Ruby NoMethodError for nil, while an empty result causes it to retry forever without making progress.

This change treats either result as TransportException::END_OF_FILE when bytes are still required. Partial reads continue normally while each call makes progress, and zero-size reads still return an empty binary string without invoking the underlying transport.

The common path returns a complete first chunk directly. A short frozen chunk is copied only when it must become the accumulator for subsequent reads.

Benchmarks

The common exact-read path was measured in the dedicated Ruby container using Ruby 4.0.6 on aarch64 Linux. A temporary stdlib benchmark used a BaseTransport whose read method returned a frozen eight-byte binary string, with 1,000,000 warmup calls followed by seven trials of 5,000,000 calls:

bundle exec ruby -Ilib /tmp/rb-read-all-benchmark.rb

  • Master c2def39207a73394420088da9b4b105571dd9036: median 6,406,402 operations/second
  • Proposed change: median 5,902,949 operations/second
  • Difference: −7.9%

This deliberately removes all underlying I/O and therefore magnifies the cost of the EOF and progress checks. Network, file, and layered transports should spend most of their time in the underlying read. Returning a complete first chunk directly avoids the allocation and copy that an intermediate accumulator would otherwise add.

  • Did you create an Apache Jira ticket? THRIFT-6144
  • If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
  • Did you squash your changes to a single commit? (not required, but preferred)
  • 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.

Client: rb

Co-Authored-By: OpenAI Codex (GPT-5.6) <codex@openai.com>
Copilot AI lite review requested due to automatic review settings August 12, 2026 13:35

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR fixes Thrift::BaseTransport#read_all in the Ruby library to properly detect end-of-input when the underlying transport returns nil or an empty string, avoiding both a leaked NoMethodError and an infinite retry loop when reads make no progress.

Changes:

  • Treat nil and empty-string reads as TransportException::END_OF_FILE when additional bytes are still required.
  • Preserve existing behavior for size == 0 (return an empty binary buffer without calling read) and for partial progress (continue accumulating while each read returns data).
  • Add spec coverage for EOF-on-no-progress cases, plus frozen first-chunk behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File Description
lib/rb/lib/thrift/transport/base_transport.rb Updates read_all to raise END_OF_FILE on nil/empty reads and to only accumulate while reads make progress (also ensures appended chunks are forced to binary).
lib/rb/spec/base_transport_spec.rb Adds focused tests covering empty/nil reads, progress behavior, frozen first chunk handling, and the zero-size read fast path.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@mergeable mergeable Bot added the ruby Pull requests that update Ruby code label Aug 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ruby Pull requests that update Ruby code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants