Make client able to use non-Send executor by Ruben2424 · Pull Request #3184 · hyperium/hyper

Ruben2424 · 2023-03-24T17:41:30Z

Hi,
I tried to solve #3017. But I got stuck.

I copied and modified existing examples to get one which fails to compile because of the missing send bounds.

The issue suggests to create a extension trait Executor<InternalFutureType<IO, B>>.
But what exactly is the InternalFutureType<IO, B>? I had the idea of a enum InternalFutureType<IO, B> with variants for every Future that gets spawned and implementing Future for it. But most the futures that get spawned are Anonymous or have Anonymous Parts.

hyper/src/proto/h2/client.rs

Line 158 in 47f614f

exec.execute(conn_task(conn, conn_drop_rx, cancel_tx));

hyper/src/proto/h2/client.rs

Line 259 in 47f614f

self.executor.execute(pipe);

hyper/src/proto/h2/client.rs

Line 316 in 47f614f

self.executor.execute(f.cb.send_when(fut));

Maybe the solution is obvious and I just don't see it? Or am I on the wrong path?
Any hint would be helpful.

seanmonstar · 2023-03-24T19:45:41Z

So, doing this is a bit unfun. Those anonymous futures would need to become structs, with manual impl Future for them. They would need to be pseudo-public. The similar one to see is proto::h2::server::H2Stream.

Then, we can add in to rt::bounds a trait that is implemented for T: Executor<ConnTask>, T: Executor<Pipe>, T: SendWhen.

Ruben2424 · 2023-05-21T15:21:57Z

I changed the anonymous futures to structs and introduced a trait. And it seams to work mostly. Some cleanup is left.
@seanmonstar is it ok to for Body to require 'static + Unpin ?

Cargo.toml

src/client/conn/http2.rs

seanmonstar · 2023-05-24T19:14:41Z

src/client/conn/http2.rs

    B::Data: Send,
    B::Error: Into<Box<dyn StdError + Send + Sync>>,
+    E: ExecutorClient<B, T> + Unpin + Clone,
+    <B as http_body::Body>::Error: std::error::Error + Send + Sync + 'static,


The line just above, B::Error: Into<..> should be enough. Is it not?

Is it better now?

Is it required to add something here at all? Because currently, this makes it stricter in that not only is it Into, but it must also be Error. That means Box<dyn Error> no longer qualifies (since Box<dyn Error>: Error is not true for confusing reasons).

src/client/conn/http2.rs

…issue-3017

Ruben2424 · 2023-06-09T13:20:00Z

I think this is ready for a second review now.

I am not 100% sure if I really understand everything I did, so please review carefully.

I got some questions:

http1 requires still Send for the io types. Is this something that needs to be addressed? And if so, in this PR?
One test didn't compile because the new bounds don't allow the error type Box<dyn std::error::Error + Send + Sync> is this something that needs to be fixed? And if how can I fix this?
I just tested the example and started the integration test. Is there something that needs to be tested manually? Or something that I need to look at when testing? Or is a testcase missing?

I hope I haven't forgotten something important here.

seanmonstar

Very impressive! I think there's just some dangling clean up that I found.

src/client/conn/http2.rs

src/common/exec.rs

src/rt/bounds.rs

seanmonstar

Very well done!

seanmonstar · 2023-06-29T13:06:32Z

Oh, looks like there's merge conflicts. Would you be up to resolving those? Then we can merge. RC4 is nearly here!

…issue-3017

Closes #3184

Closes hyperium#3017 BREAKING CHANGE: `client::conn::http2` types now use another generic for an `Executor`. Code that names `Connection` needs to include the additional generic parameter.

Closes hyperium#3184

Closes hyperium#3017 BREAKING CHANGE: `client::conn::http2` types now use another generic for an `Executor`. Code that names `Connection` needs to include the additional generic parameter. Signed-off-by: Sven Pfennig <[email protected]>

Closes hyperium#3184 Signed-off-by: Sven Pfennig <[email protected]>

create example

d88ec30

make anonymous futures structs

44587d6

seanmonstar reviewed May 24, 2023

View reviewed changes

Ruben2424 and others added 17 commits June 6, 2023 21:06

remove exec and fix warnings

55753a9

remove bound

ad57f1c

fmt

4132aa6

use right future

4138669

fix features ci

ce4619e

fix ffi

4ac835a

Merge branch 'master' into issue-3017

27eb1e2

move client example to single_threaded example

ed95666

Merge branch 'issue-3017' of https://github.com/Ruben2424/hyper into …

beb3ee4

…issue-3017

remove from cargo toml

c4a51b3

use pin_project_lite

7cbfc72

deny warnings

d3821a9

error bounds

2394949

fix test

8e0f907

sealed ExecutorClient

6d44c29

better error message

c7d59fe

make it work also for io types

0269396

Ruben2424 marked this pull request as ready for review June 9, 2023 12:50

Ruben2424 requested a review from seanmonstar June 9, 2023 13:20

Ruben2424 added 2 commits June 12, 2023 23:45

improve example

3a78a57

fmt

be5c654

Ruben2424 mentioned this pull request Jun 14, 2023

Bump msrv to 1.60 #3250

Closed

seanmonstar reviewed Jun 16, 2023

View reviewed changes

Merge branch 'master' into issue-3017

b39d5d4

Ruben2424 added 2 commits June 16, 2023 22:54

fix merge fail

15be46f

fix error bounds

8391604

Ruben2424 requested a review from seanmonstar June 17, 2023 12:57

Merge branch 'master' into issue-3017

a276efc

seanmonstar changed the title ~~WIP: Make client able to use non-Send executor~~ Make client able to use non-Send executor Jun 26, 2023

seanmonstar linked an issue Jun 29, 2023 that may be closed by this pull request

Make client able to use non-Send executor #3017

Closed

seanmonstar approved these changes Jun 29, 2023

View reviewed changes

Ruben2424 added 3 commits June 29, 2023 18:24

Merge branch 'master' into issue-3017

3dd0579

Merge branch 'issue-3017' of https://github.com/Ruben2424/hyper into …

98b0599

…issue-3017

Merge branch 'master' into issue-3017

25ff95c

seanmonstar merged commit d977f20 into hyperium:master Jun 29, 2023

Ruben2424 mentioned this pull request Jul 14, 2023

remove send bounds and add example also for http1 #3266

Merged

seanmonstar pushed a commit that referenced this pull request Jul 18, 2023

fix(client): remove Send bounds for request Body (#3266)

4ace340

Closes #3184

This was referenced Aug 18, 2023

h2/client(1.0.0-rc4): panic on a double poll on Select #3289

Closed

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

Merged

0xE282B0 pushed a commit to 0xE282B0/hyper that referenced this pull request Jan 12, 2024

fix(client): remove Send bounds for request Body (hyperium#3266)

2356a44

Closes hyperium#3184

0xE282B0 pushed a commit to 0xE282B0/hyper that referenced this pull request Jan 16, 2024

fix(client): remove Send bounds for request Body (hyperium#3266)

5400295

Closes hyperium#3184 Signed-off-by: Sven Pfennig <[email protected]>

sattva9 mentioned this pull request Feb 27, 2024

hyper-1.0 client and server examples DataDog/glommio#639

Merged

Uh oh!

Conversation

Ruben2424 commented Mar 24, 2023

Uh oh!

seanmonstar commented Mar 24, 2023

Uh oh!

Ruben2424 commented May 21, 2023

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seanmonstar May 24, 2023

Choose a reason for hiding this comment

Uh oh!

Ruben2424 Jun 6, 2023

Choose a reason for hiding this comment

Uh oh!

seanmonstar Jun 15, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Ruben2424 commented Jun 9, 2023

Uh oh!

seanmonstar left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

seanmonstar left a comment

Choose a reason for hiding this comment

Uh oh!

seanmonstar commented Jun 29, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants