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

Make client able to use non-Send executor #3184

Merged
merged 28 commits into from
Jun 29, 2023
Merged

Make client able to use non-Send executor #3184

merged 28 commits into from
Jun 29, 2023

Conversation

Ruben2424
Copy link
Contributor

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
Copy link
Member

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

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 ?

seanmonstar
seanmonstar reviewed May 24, 2023
View reviewed changes
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
src/client/conn/http2.rs Outdated Show resolved Hide resolved
src/client/conn/http2.rs Outdated
where
E: Executor<BoxSendFuture> + Send + Sync + 'static,
T: AsyncRead + AsyncWrite + Unpin + Send + 'static,
B: Body + 'static,
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,
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it better now?

Copy link
Member

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
src/client/conn/http2.rs Outdated Show resolved Hide resolved
@Ruben2424 Ruben2424 marked this pull request as ready for review June 9, 2023 12:50
@Ruben2424
Copy link
Contributor Author

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:

  1. http1 requires still Send for the io types. Is this something that needs to be addressed? And if so, in this PR?
  2. 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?
  3. 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.

@Ruben2424 Ruben2424 mentioned this pull request Jun 14, 2023
Closed
seanmonstar
seanmonstar reviewed Jun 16, 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.

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

src/client/conn/http2.rs Outdated Show resolved Hide resolved
src/client/conn/http2.rs Outdated Show resolved Hide resolved
src/client/conn/http2.rs Outdated Show resolved Hide resolved
src/common/exec.rs Show resolved Hide resolved
src/rt/bounds.rs Outdated Show resolved Hide resolved
src/rt/bounds.rs Outdated Show resolved Hide resolved
@seanmonstar 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 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
seanmonstar approved these changes Jun 29, 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.

Very well done!

@seanmonstar
Copy link
Member

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

@seanmonstar seanmonstar merged commit d977f20 into hyperium:master Jun 29, 2023
17 checks passed
@Ruben2424 Ruben2424 mentioned this pull request Jul 14, 2023
Merged
seanmonstar pushed a commit that referenced this pull request Jul 18, 2023
@Ruben2424
fix(client): remove Send bounds for request Body (#3266)
4ace340 
Closes #3184
This was referenced Aug 18, 2023
Closed
Merged
0xE282B0 pushed a commit to 0xE282B0/hyper that referenced this pull request Jan 12, 2024
@Ruben2424 @0xE282B0
feat(client): Make clients able to use non-Send executor (hyperium#3184)
e38f6d3 
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.
0xE282B0 pushed a commit to 0xE282B0/hyper that referenced this pull request Jan 12, 2024
@Ruben2424 @0xE282B0
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
@Ruben2424 @0xE282B0
feat(client): Make clients able to use non-Send executor (hyperium#3184)
eb28641 
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 <s.pfennig@reply.de>
0xE282B0 pushed a commit to 0xE282B0/hyper that referenced this pull request Jan 16, 2024
@Ruben2424 @0xE282B0
fix(client): remove Send bounds for request Body (hyperium#3266)
5400295 
Closes hyperium#3184

Signed-off-by: Sven Pfennig <s.pfennig@reply.de>
@sattva9 sattva9 mentioned this pull request Feb 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@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.

Make client able to use non-Send executor
2 participants
@Ruben2424 @seanmonstar