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.

Sign up for GitHub

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

Jump to bottom

fix(http2/client): avoid double-polling a Select future #3290

Merged
merged 3 commits into from
Aug 21, 2023
Merged

fix(http2/client): avoid double-polling a Select future #3290

merged 3 commits into from
Aug 21, 2023

Conversation

lucab
Copy link
Contributor

@lucab lucab commented Aug 18, 2023

This reworks http2 client connection task in order to avoid storing a Select future. Under some scheduling cases the future was polled multiple times, resulting in runtime panics:

thread 'main' panicked at 'cannot poll Select twice', /index.crates.io/futures-util-0.3.28/src/future/select.rs:112:42
stack backtrace:
[...]
   5: <futures_util::future::select::Select<A,B> as core::future::future::Future>::poll
   6: <hyper::proto::h2::client::H2ClientFuture<B,T> as core::future::future::Future>::poll
[...]

Closes: #3289

@lucab
fix(http2/client): avoid double-polling a Select future
bd9443e 
This reworks http2 client connection task in order to avoid storing
a `Select` future. Under some scheduling cases the future was polled
multiple times, resulting in runtime panics:

```
thread 'main' panicked at 'cannot poll Select twice', /index.crates.io/futures-util-0.3.28/src/future/select.rs:112:42
stack backtrace:
[...]
   5: <futures_util::future::select::Select<A,B> as core::future::future::Future>::poll
   6: <hyper::proto::h2::client::H2ClientFuture<B,T> as core::future::future::Future>::poll
[...]
```
@lucab lucab mentioned this pull request Aug 18, 2023
Closed
@lucab
Copy link
Contributor Author

lucab commented Aug 18, 2023

@seanmonstar unfortunately I've only observed these panics on a remote service, but I didn't manage to easily reproduce it manually or through a test.
We did run this patch on the staging environment and didn't observe panics, but maybe it was just a lucky run where we didn't hit the same race condition.

I put together this patch based on the stacktrace and some educated guesses, but I'm not really familiar with this codebase.

The code was introduced in #3184 by @Ruben2424, you may want to double-check this.

Ruben2424
Ruben2424 reviewed Aug 19, 2023
View reviewed changes
Copy link
Contributor

@Ruben2424 Ruben2424 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 the PR!
All in all, I'm not sure if this truly addresses the underlying issue or merely treats the symptoms.
As of my understanding the future should never be polled twice.

src/proto/h2/client.rs Show resolved Hide resolved
@lucab
Copy link
Contributor Author

lucab commented Aug 20, 2023

Alternatively I think we could also keep the Select (which already implements FusedFuture) and perform the is_terminated() check on it before polling.
But honestly I think that getting rid of it makes the ConnTask::poll() logic simpler to follow.

seanmonstar
seanmonstar approved these changes Aug 21, 2023
View reviewed changes
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.

Makes sense, thanks for catching and providing the fix! <3

@seanmonstar seanmonstar merged commit fece9f7 into hyperium:master Aug 21, 2023
17 checks passed
sfackler
sfackler reviewed Aug 21, 2023
View reviewed changes
src/proto/h2/client.rs
@@ -218,6 +221,8 @@ pin_project! {
{
#[pin]
conn: Either<Conn<T, B>, Connection<Compat<T>, SendBuf<<B as Body>::Data>>>,
#[pin]
Copy link
Contributor

Choose a reason for hiding this comment

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

No need to pin a bool.

lucab added a commit to lucab/hyper that referenced this pull request Aug 22, 2023
@lucab
fix(client): avoid double-polling a Select future (hyperium#3290)
3a16dbf 
This reworks http2 client connection task in order to avoid storing
a `Select` future. Under some scheduling cases the future was polled
multiple times, resulting in runtime panics:

```
thread 'main' panicked at 'cannot poll Select twice', /index.crates.io/futures-util-0.3.28/src/future/select.rs:112:42
stack backtrace:
[...]
   5: <futures_util::future::select::Select<A,B> as core::future::future::Future>::poll
   6: <hyper::proto::h2::client::H2ClientFuture<B,T> as core::future::future::Future>::poll
[...]
```

Closes hyperium#3289
@lucab lucab mentioned this pull request Aug 22, 2023
Merged
kdorosh pushed a commit to kdorosh/hyper that referenced this pull request Sep 13, 2023
@lucab
fix(client): avoid double-polling a Select future (hyperium#3290)
26b66d2 
This reworks http2 client connection task in order to avoid storing
a `Select` future. Under some scheduling cases the future was polled
multiple times, resulting in runtime panics:

```
thread 'main' panicked at 'cannot poll Select twice', /index.crates.io/futures-util-0.3.28/src/future/select.rs:112:42
stack backtrace:
[...]
   5: <futures_util::future::select::Select<A,B> as core::future::future::Future>::poll
   6: <hyper::proto::h2::client::H2ClientFuture<B,T> as core::future::future::Future>::poll
[...]
```

Closes hyperium#3289
kdorosh pushed a commit to solo-io/hyper that referenced this pull request Sep 13, 2023
@lucab
fix(client): avoid double-polling a Select future (hyperium#3290)
ed486b7 
This reworks http2 client connection task in order to avoid storing
a `Select` future. Under some scheduling cases the future was polled
multiple times, resulting in runtime panics:

```
thread 'main' panicked at 'cannot poll Select twice', /index.crates.io/futures-util-0.3.28/src/future/select.rs:112:42
stack backtrace:
[...]
   5: <futures_util::future::select::Select<A,B> as core::future::future::Future>::poll
   6: <hyper::proto::h2::client::H2ClientFuture<B,T> as core::future::future::Future>::poll
[...]
```

Closes hyperium#3289
0xE282B0 pushed a commit to 0xE282B0/hyper that referenced this pull request Jan 12, 2024
@lucab @0xE282B0
fix(client): avoid double-polling a Select future (hyperium#3290)
4e929bc 
This reworks http2 client connection task in order to avoid storing
a `Select` future. Under some scheduling cases the future was polled
multiple times, resulting in runtime panics:

```
thread 'main' panicked at 'cannot poll Select twice', /index.crates.io/futures-util-0.3.28/src/future/select.rs:112:42
stack backtrace:
[...]
   5: <futures_util::future::select::Select<A,B> as core::future::future::Future>::poll
   6: <hyper::proto::h2::client::H2ClientFuture<B,T> as core::future::future::Future>::poll
[...]
```

Closes hyperium#3289
0xE282B0 pushed a commit to 0xE282B0/hyper that referenced this pull request Jan 16, 2024
@lucab @0xE282B0
fix(client): avoid double-polling a Select future (hyperium#3290)
7d3fddd 
This reworks http2 client connection task in order to avoid storing
a `Select` future. Under some scheduling cases the future was polled
multiple times, resulting in runtime panics:

```
thread 'main' panicked at 'cannot poll Select twice', /index.crates.io/futures-util-0.3.28/src/future/select.rs:112:42
stack backtrace:
[...]
   5: <futures_util::future::select::Select<A,B> as core::future::future::Future>::poll
   6: <hyper::proto::h2::client::H2ClientFuture<B,T> as core::future::future::Future>::poll
[...]
```

Closes hyperium#3289

Signed-off-by: Sven Pfennig <s.pfennig@reply.de>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ruben2424 Ruben2424 Ruben2424 left review comments

@sfackler sfackler sfackler left review comments

@piscisaureus piscisaureus piscisaureus left review comments

@seanmonstar seanmonstar seanmonstar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

h2/client(1.0.0-rc4): panic on a double poll on Select
5 participants
@lucab @seanmonstar @piscisaureus @sfackler @Ruben2424