Skip to content

Commit d9689ea

Browse files
authored
fix: Account for connection flow control on DATA after GOAWAY (#895)
See: * Golang: https://github.com/golang/net/blob/0a81d5af911d6b2c1ad6c271c6aa2fe09747fd75/http2/server.go#L1440-L1453 * nghttp2: https://github.com/nghttp2/nghttp2/blob/1f8481251cb28848b45d950de6cad6a1311eedf5/lib/nghttp2_session.c#L6935-L6960 When h2 sends GOAWAY and then receives DATA on a stream above the GOAWAY `last_stream_id`, the data is correctly ignored, but flow control capacity is leaked at the connection level. This can cause an issue with streams lower than `last_stream_id` being stalled forever.
1 parent 08129b2 commit d9689ea

2 files changed

Lines changed: 118 additions & 1 deletion

File tree

src/proto/streams/streams.rs

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -555,6 +555,13 @@ impl Inner {
555555
id,
556556
self.actions.recv.max_stream_id()
557557
);
558+
559+
// We still need to account for connection-level flow control.
560+
let sz = frame.flow_controlled_len();
561+
assert!(sz <= super::MAX_WINDOW_SIZE as usize);
562+
let sz = sz as WindowSize;
563+
self.actions.recv.ignore_data(sz)?;
564+
558565
return Ok(());
559566
}
560567

@@ -566,8 +573,8 @@ impl Inner {
566573
// this is just a sanity check.
567574
assert!(sz <= super::MAX_WINDOW_SIZE as usize);
568575
let sz = sz as WindowSize;
569-
570576
self.actions.recv.ignore_data(sz)?;
577+
571578
return Err(Error::library_reset(id, Reason::STREAM_CLOSED));
572579
}
573580

tests/h2-tests/tests/flow_control.rs

Lines changed: 110 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2453,3 +2453,113 @@ async fn too_many_window_update_resets_causes_go_away() {
24532453

24542454
join(srv, client).await;
24552455
}
2456+
2457+
#[tokio::test]
2458+
async fn goaway_ignores_data_but_returns_connection_capacity() {
2459+
h2_support::trace_init!();
2460+
2461+
for padded in [false, true] {
2462+
let (io, mut client) = mock::new();
2463+
2464+
// Test both with and without padding
2465+
let data = if padded {
2466+
let mut frame = vec![0u8; 16_384];
2467+
frame[0] = 5; // 5 bytes of padding
2468+
frame
2469+
} else {
2470+
vec![0u8; 16_384]
2471+
};
2472+
2473+
let client = async move {
2474+
let _ = client.assert_server_handshake().await;
2475+
2476+
client
2477+
.send_frame(
2478+
frames::headers(1)
2479+
.request("GET", "https://example.com/")
2480+
.eos(),
2481+
)
2482+
.await;
2483+
2484+
// Receive GOAWAY(MAX) + PING from graceful shutdown.
2485+
client.recv_frame(frames::go_away(2147483647)).await;
2486+
client.recv_frame(frames::ping(frame::Ping::SHUTDOWN)).await;
2487+
2488+
client
2489+
.recv_frame(frames::headers(1).response(200).eos())
2490+
.await;
2491+
2492+
// Stream 3 arrives "in flight" before client processes GOAWAY.
2493+
client
2494+
.send_frame(frames::headers(3).request("POST", "https://example.com/"))
2495+
.await;
2496+
2497+
// Complete the graceful shutdown handshake.
2498+
client
2499+
.send_frame(frames::ping(frame::Ping::SHUTDOWN).pong())
2500+
.await;
2501+
2502+
// Final GOAWAY(3): streams 1 and 3 are accepted, everything above is rejected.
2503+
client.recv_frame(frames::go_away(3)).await;
2504+
2505+
// Stream 5 is above last_stream_id=3; DATA will be ignored,
2506+
// but connection window must still be replenished.
2507+
client
2508+
.send_frame(frames::headers(5).request("POST", "https://example.com/"))
2509+
.await;
2510+
client
2511+
.send_frame(if padded {
2512+
frames::data(5, &data[..]).padded()
2513+
} else {
2514+
frames::data(5, &data[..])
2515+
})
2516+
.await;
2517+
client
2518+
.send_frame(if padded {
2519+
frames::data(5, &data[..]).padded()
2520+
} else {
2521+
frames::data(5, &data[..])
2522+
})
2523+
.await;
2524+
2525+
client
2526+
.recv_frame(frames::window_update(0, 16_384 * 2))
2527+
.await;
2528+
2529+
client.send_frame(frames::data(3, "").eos()).await;
2530+
2531+
client
2532+
.recv_frame(frames::headers(3).response(200).eos())
2533+
.await;
2534+
2535+
client.recv_eof().await;
2536+
};
2537+
2538+
let srv = async move {
2539+
let mut srv = server::handshake(io).await.expect("handshake");
2540+
2541+
let (_req, mut stream) = srv.next().await.unwrap().unwrap();
2542+
srv.graceful_shutdown();
2543+
let rsp = http::Response::builder().status(200).body(()).unwrap();
2544+
stream.send_response(rsp, true).unwrap();
2545+
2546+
let (req, mut stream) = srv.next().await.unwrap().unwrap();
2547+
let body = req.into_parts().1;
2548+
2549+
let body = async move {
2550+
let buf = util::concat(body).await.unwrap();
2551+
assert!(buf.is_empty());
2552+
let rsp = http::Response::builder().status(200).body(()).unwrap();
2553+
stream.send_response(rsp, true).unwrap();
2554+
};
2555+
2556+
let mut srv = Box::pin(async move {
2557+
assert!(srv.next().await.is_none(), "unexpected stream after GOAWAY");
2558+
});
2559+
srv.drive(body).await;
2560+
srv.await;
2561+
};
2562+
2563+
join(client, srv).await;
2564+
}
2565+
}

0 commit comments

Comments
 (0)