Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

large multi-chunk GRPC responses get stuck #24552

Closed
rnbguy opened this issue Jul 12, 2024 · 0 comments · Fixed by #24576
Closed

large multi-chunk GRPC responses get stuck #24552

rnbguy opened this issue Jul 12, 2024 · 0 comments · Fixed by #24576
Labels
bug Something isn't working correctly node compat

Comments

@rnbguy
Copy link
Contributor

rnbguy commented Jul 12, 2024

Version: Deno 1.45.1

If a response has more than u16::MAX many bytes, these polls get stuck.

deno/ext/node/ops/http2.rs

Lines 455 to 477 in 8cbf81c

fn poll_data_or_trailers(
cx: &mut std::task::Context,
body: &mut RecvStream,
) -> Poll<Result<DataOrTrailers, h2::Error>> {
loop {
if let Poll::Ready(trailers) = body.poll_trailers(cx) {
if let Some(trailers) = trailers? {
return Poll::Ready(Ok(DataOrTrailers::Trailers(trailers)));
} else {
return Poll::Ready(Ok(DataOrTrailers::Eof));
}
}
if let Poll::Ready(data) = body.poll_data(cx) {
if let Some(data) = data {
return Poll::Ready(Ok(DataOrTrailers::Data(data?)));
}
// If data is None, loop one more time to check for trailers
continue;
}
// Return pending here as poll_data will keep the waker
return Poll::Pending;
}
}

I created a simple pingpong project to reproduce this using a python grpc server: https://github.com/rnbguy/deno-grpc-multi-chunk

Using this example, I realized, Deno parses u16::MAX many bytes in four chunks - 16_384, 16_384, 16_384 and 16_383 - then the poll gets stuck if there are more bytes pending.

Ref: #24147 (comment)

@rnbguy rnbguy changed the title multi-chunk GRPC response gets stuck large multi-chunk GRPC responses get stuck Jul 13, 2024
@satyarohith satyarohith added bug Something isn't working correctly node compat labels Jul 15, 2024
bartlomieju pushed a commit that referenced this issue Jul 22, 2024
This PR adds logic to release window capacity after reading the chunks
from the stream. Without it, large response (more than `u16::MAX`) may
fill up the capacity and the whole response can't be read.

Closes #24552
Closes #24305
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly node compat
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants