Conversation
|
|
When a call completes, destroyHttp2Stream() invokes http2Stream.end() if the server ended the call. However, for unary and server-streaming calls (as well as client streams where writing already finished), halfClose() has already ended the stream. In Node.js, calling .end() on an already finished stream without a callback causes Node's stream internals to construct an ERR_STREAM_ALREADY_FINISHED Error with a full V8 stack trace, only to immediately discard it. Under high throughput, this introduces significant CPU overhead and garbage collection pressure. This change checks writableEnded before calling http2Stream.end() in destroyHttp2Stream() and halfClose(), ensuring .end() is only called when the client side of the stream has not yet been closed.
olavloite
force-pushed
the
avoid-redundant-end
branch
from
September 13, 2026 16:38
01934c6 to
dffda21
Compare
olavloite
added a commit
to googleapis/google-cloud-node
that referenced
this pull request
Sep 13, 2026
In @grpc/grpc-js, destroyHttp2Stream() calls http2Stream.end() when the server finishes a call, even if the stream was already ended by halfClose(). Calling .end() on an already ended or destroyed Node stream without a callback causes Node core to construct an ERR_STREAM_ALREADY_FINISHED or ERR_STREAM_DESTROYED error with a full V8 stack capture and immediately discard it, creating unnecessary CPU overhead on high-throughput workloads. This adds a workaround on stream.Duplex.prototype.end scoped to HTTP/2 streams that returns early when called on an already ended or destroyed stream without data or a callback. The workaround can be disabled with SPANNER_DISABLE_HTTP2_STREAM_END_WORKAROUND=true. This is a temporary client-side workaround for an upstream issue in @grpc/grpc-js, tracked in grpc/grpc-node#3082. It can be removed once Spanner requires a grpc-js release that includes that fix.
olavloite
added a commit
to googleapis/google-cloud-node
that referenced
this pull request
Sep 14, 2026
In @grpc/grpc-js, destroyHttp2Stream() calls http2Stream.end() when the server finishes a call, even if the stream was already ended by halfClose(). Calling .end() on an already ended or destroyed Node stream without a callback causes Node core to construct an ERR_STREAM_ALREADY_FINISHED or ERR_STREAM_DESTROYED error with a full V8 stack capture and immediately discard it, creating unnecessary CPU overhead on high-throughput workloads. This adds a workaround on stream.Duplex.prototype.end scoped to HTTP/2 streams that returns early when called on an already ended or destroyed stream without data or a callback. The workaround can be disabled with SPANNER_DISABLE_HTTP2_STREAM_END_WORKAROUND=true. This is a temporary client-side workaround for an upstream issue in @grpc/grpc-js, tracked in grpc/grpc-node#3082. It can be removed once Spanner requires a grpc-js release that includes that fix.
Member
|
The new test code doesn't compile |
Add explicit type cast for the intercepted HTTP/2 stream and type arguments for makeClientStreamRequest.
Author
Sorry, that should be fixed now. |
Author
|
The current build failures seem to be unrelated to this change: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a call completes, destroyHttp2Stream() invokes http2Stream.end() if the server ended the call. However, for unary and server-streaming calls (as well as client streams where writing already finished), halfClose() has already ended the stream.
In Node.js, calling .end() on an already finished stream without a callback causes Node's stream internals to construct an ERR_STREAM_ALREADY_FINISHED Error with a full V8 stack trace, only to immediately discard it. Under high throughput, this introduces significant CPU overhead and garbage collection pressure.
This change checks writableEnded before calling http2Stream.end() in destroyHttp2Stream() and halfClose(), ensuring .end() is only called when the client side of the stream has not yet been closed.