Skip to content

perf(spanner): replace setTimeout backpressure with event-driven resumption - #9351

Open
olavloite wants to merge 1 commit into
mainfrom
spanner-fix-back-pressure
Open

olavloite wants to merge 1 commit into
mainfrom
spanner-fix-back-pressure

Conversation

@olavloite

Copy link
Copy Markdown
Contributor

Replace the setTimeout backpressure polling loop in PartialResultStream with native Node.js stream flow control:

  • Hold the transform completion callback when downstream push() returns false, and invoke it upon stream drain via _read() to resume upstream ingestion.
  • Remove _tryResume, _numPushFailed, and the artificial 'Stream is still not ready to receive data' timeout failures.
  • Guard 'paused' event emission in _addChunk so it only emits once per backpressure transition instead of on every subsequent value.
  • Mark maxResumeRetries as deprecated on streaming RowOptions, clarifying that it is only used by non-streaming Snapshot.run() for RPC retry limits.
  • Replace setTimeout with setImmediate in test/spanner.ts slow-writer test, eliminating a 5-second sleep in compliance with the zero-sleep test policy.
  • Add comprehensive backpressure unit tests covering single-chunk direct decoding, multi-chunk pause/resume cycles, cross-chunk value stitching, stream destroy/error propagation, and full-pipeline flow control.

…mption

Replace the setTimeout backpressure polling loop in PartialResultStream with
native Node.js stream flow control:
- Hold the transform completion callback when downstream push() returns false,
  and invoke it upon stream drain via _read() to resume upstream ingestion.
- Remove _tryResume, _numPushFailed, and the artificial 'Stream is still not
  ready to receive data' timeout failures.
- Guard 'paused' event emission in _addChunk so it only emits once per
  backpressure transition instead of on every subsequent value.
- Mark maxResumeRetries as deprecated on streaming RowOptions, clarifying that
  it is only used by non-streaming Snapshot.run() for RPC retry limits.
- Replace setTimeout with setImmediate in test/spanner.ts slow-writer test,
  eliminating a 5-second sleep in compliance with the zero-sleep test policy.
- Add comprehensive backpressure unit tests covering single-chunk direct
  decoding, multi-chunk pause/resume cycles, cross-chunk value stitching,
  stream destroy/error propagation, and full-pipeline flow control.
@olavloite
olavloite requested a review from a team as a code owner September 16, 2026 13:06
@product-auto-label product-auto-label Bot added the api: spanner Issues related to the Spanner API. label Sep 16, 2026

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request replaces the timeout-based retry mechanism for stream backpressure with automatic, event-driven stream flow control in PartialResultStream, deprecating the maxResumeRetries option for streaming queries. Feedback on the changes suggests wrapping super._read(size) in an else block inside the _read method to avoid potential redundant calls to the parent class's read implementation when resuming from a paused state.

Comment thread handwritten/spanner/src/partial-result-stream.ts
@olavloite

Copy link
Copy Markdown
Contributor Author

/gemini review

@gemini-code-assist gemini-code-assist Bot 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.

Code Review

This pull request refactors backpressure handling in PartialResultStream to use native Node.js stream flow control instead of a timer-based retry mechanism. The maxResumeRetries option is deprecated for streaming queries, and the stream now pauses and resumes automatically using _read() and a cached callback when downstream buffers are full. Comprehensive unit tests have been added to verify this event-driven backpressure behavior, and existing tests have been updated to reflect that slow writers no longer trigger failures. There are no review comments to address, and the implementation looks solid.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: spanner Issues related to the Spanner API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant