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

fix: panic in pop_frame() #657

Merged
merged 1 commit into from
Feb 13, 2023
Merged

Conversation

aftersnow
Copy link
Contributor

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
Copy link
Contributor Author

#607

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so so much for sending a PR! It's always appreciated when people try to help fix the problems they find :)

src/proto/streams/flow_control.rs Show resolved Hide resolved
src/proto/streams/flow_control.rs Outdated Show resolved Hide resolved
src/proto/streams/prioritize.rs Outdated Show resolved Hide resolved
@aftersnow
Copy link
Contributor Author

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

@seanmonstar
Copy link
Member

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?

    We met the panic in our production environment, so handle this panic
    condition before panic. The 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
Copy link
Contributor Author

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.

Copy link
Member

@seanmonstar seanmonstar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for keeping up with this, LGTM!

@seanmonstar seanmonstar merged commit 73bea23 into hyperium:master Feb 13, 2023
@aftersnow aftersnow deleted the pop_frame_panic branch February 14, 2023 03:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants