Skip to content

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

Merged
seanmonstar merged 3 commits intohyperium:masterfrom
lucab:ups/client-select-panic
Aug 21, 2023
Merged

fix(http2/client): avoid double-polling a Select future#3290
seanmonstar merged 3 commits intohyperium:masterfrom
lucab:ups/client-select-panic

Conversation

@lucab
Copy link
Copy Markdown
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
Copy Markdown
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.

piscisaureus
piscisaureus reviewed Aug 18, 2023
View reviewed changes
piscisaureus
piscisaureus reviewed Aug 18, 2023
View reviewed changes
Ruben2424
Ruben2424 reviewed Aug 19, 2023
View reviewed changes
Copy link
Copy Markdown
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.

@lucab
Copy link
Copy Markdown
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
Copy Markdown
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
sfackler
sfackler reviewed Aug 21, 2023
View reviewed changes
src/proto/h2/client.rs
{
#[pin]
conn: Either<Conn<T, B>, Connection<Compat<T>, SendBuf<<B as Body>::Data>>>,
#[pin]
Copy link
Copy Markdown
Contributor

@sfackler sfackler Aug 21, 2023

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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sfackler sfackler sfackler left review comments

@seanmonstar seanmonstar seanmonstar approved these changes

+2 more reviewers

@piscisaureus piscisaureus piscisaureus left review comments

@Ruben2424 Ruben2424 Ruben2424 left review comments

Reviewers whose approvals may not affect merge requirements

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