hyperium / h2

HTTP 2.0 client & server implementation for Rust.
MIT License
1.34k stars 269 forks source link

fix: panic in pop_frame() #657

Closed aftersnow closed 1 year ago

aftersnow commented 1 year ago
We met the panic in our production environment, so handle this panic
condition before panic. stack backtrace:
   0: rust_begin_unwind
             at
/rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/std/src/panicking.rs:517:5
   1: core::panicking::panic_fmt
             at
/rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/core/src/panicking.rs:101:14
   2: core::panicking::panic
             at
/rustc/59eed8a2aac0230a8b53e89d4e99d55912ba6b35/library/core/src/panicking.rs:50:5
   3: h2::proto::streams::flow_control::FlowControl::send_data
             at
/build/vendor/h2/src/proto/streams/flow_control.rs:176:9
   4: h2::proto::streams::prioritize::Prioritize::pop_frame::{{closure}}
             at
/build/vendor/h2/src/proto/streams/prioritize.rs:737:33
   5: tracing::span::Span::in_scope
             at /build/vendor/tracing/src/span.rs:982:9
   6: h2::proto::streams::prioritize::Prioritize::pop_frame
             at
/build/vendor/h2/src/proto/streams/prioritize.rs:736:29
   7: h2::proto::streams::prioritize::Prioritize::poll_complete
             at
/build/vendor/h2/src/proto/streams/prioritize.rs:497:19
   8: h2::proto::streams::send::Send::poll_complete
             at /build/vendor/h2/src/proto/streams/send.rs:297:9
   9: h2::proto::streams::streams::Inner::poll_complete
             at /build/vendor/h2/src/proto/streams/streams.rs:850:16
  10: h2::proto::streams::streams::Streams<B,P>::poll_complete
             at /build/vendor/h2/src/proto/streams/streams.rs:180:9
  11: h2::proto::connection::Connection<T,P,B>::poll
             at /build/vendor/h2/src/proto/connection.rs:253:36
  12: <h2::client::Connection<T,B> as
core::future::future::Future>::poll
  13: ...
aftersnow commented 1 year ago

607

aftersnow commented 1 year ago

@seanmonstar Hi, have changed the PR, could you review the PR again please?

seanmonstar commented 1 year ago

Sorry for forgetting about this. I think fixing this issue is a really nice improvement. I wonder if adding a unit test making sure we never regress this would be worth doing. I suppose the instance is a case where perhaps the connection window size is larger than the accumulative stream windows, right? What do you think? Does it look manageable to add a test case?

Have you, by chance, tried out this patch in your application to see it fixed the panics?

aftersnow commented 1 year ago

OK, I fixed the test error: If the frame length equals the windows size, it should not be pushed front. Now all tests passed. (Maybe the current UTs are enough?)

I suppose the instance is a case where perhaps the connection window size is larger than the accumulative stream windows, right?

Yes, I found the error in our production environment (see the commit message). But I am not familiar with the http protocol, so I cannot explain it deeply. Do you have any suggestions for me?

Does it look manageable to add a test case?

It would be better to add a test case, but I don't know how to reproduce the case...

Have you, by chance, tried out this patch in your application to see it fixed the panics?

Yes, the initial commit of this PR has been tested in our production environment, and fixed the panic.