Skip to content

feat(client, server): remove the tcp cargo feature and related code #2878

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
Closed
wants to merge 19 commits into from
Closed

feat(client, server): remove the tcp cargo feature and related code #2878

wants to merge 19 commits into from

Conversation

MrGunflame
Copy link
Contributor

@MrGunflame MrGunflame commented May 27, 2022 •
edited
Loading

Removes the tcp cargo feature and related code from server and client. I've kept client::connect even thought it doesn't serve much use without tcp-related features.

Closes #2856.

MrGunflame added 4 commits May 26, 2022 20:23
@MrGunflame
feat(server): remove tcp feature gated server code
7551d26 
remove all tcp feature gated code in the server module

partially closes hyperium#2856
@MrGunflame
feat(server): remove failing examples using bind
1b369a2 
removes the examples for Builder::serve and
Server::with_graceful_shutdown which are failing after the Server::bind
is removed

partially closes hyperium#2856
@MrGunflame
feat(client): remove tcp dependant client code
157030f 
remove client code that depends on tcp features. Also removes several
tests and doc examples that are broken without the tcp feature

partially closes hyperium#2856
@MrGunflame
feat(server,client): remove the tcp feature
7bb1f8f 
remove the tcp feature from Cargo.toml

partially closes hyperium#2856
@seanmonstar
Copy link
Member

Woah, awesome work, thanks!

@MrGunflame
feat(client): remove unused types and imports
bc3a1ca 
remove unused types from the client connect module and fix unused
import warnings
@MrGunflame
Copy link
Contributor Author

It should be noted that this change currently breaks pretty much every test and example, which all rely on the Server/Client type.

@seanmonstar
Copy link
Member

Alright, the meta pieces are in place now, so full-speed ahead on the feature work...

Hm, it looks like we'll need some sort of local utility for the unit tests...

@MrGunflame
Copy link
Contributor Author

Yes, I think all tcp feature code is gone now. I've been working on converting the tests to use conn client/server mod, but I'm not quite happy with it. The examples also pose a similar problem.

@seanmonstar
Copy link
Member

I'm looking through the tests right now, and it seems like a good time to clean up this mess anyways 😅 . Most of the client test! stuff can probably have the inner part of the macro just use conn and TcpStream instead of Client. The conn impl tests are probably fine as-is.

... As I scroll more, I'm not sure there's many tests that are checking the higher-level Client specifically... If they are, we can comment them out and move them to the https://github.com/hyperium/hyper-util crate as part of the Client move.

@MrGunflame
feat(client,server): Replace tcp feature dependent tests
bfef663 
add the `net` feature to `tokio` dependencies. Replace all usages of
Server and Client with locally created types implemented using
TcpListener/TcpStream and the server/client conn module
@seanmonstar
Copy link
Member

Aww, sorry, it looks like merging #2896 caused conflicts with this 🙈

@MrGunflame
Copy link
Contributor Author

That's fine, I'll have to update the examples anyways before this works.

@Robert-Cunningham
Copy link
Contributor

When we merge this PR, will Exec struct in common/exec.rs have a purpose anymore? Or will we then want to replace Exec everywhere Box<dyn Executor> (and simultaneously add TokioExecutor to hyper-utils)?

@MrGunflame
Copy link
Contributor Author

I think you have an argument there to replace Exec with an Arc<dyn Executor>, but I'll likely just keep change the Exec enum into a struct (and remove the Default variant).

@MrGunflame
Copy link
Contributor Author

All tests except get_strip_connection_header should pass now. For some reason get_strip_connection_header gets stuck when using the proxy connection and I haven't been able to find out why that is yet.

@seanmonstar
Copy link
Member

Thanks for plugging away at this! I'll kick off another CI run.

@MrGunflame
Copy link
Contributor Author

I believe it should finally pass now.

@seanmonstar
Copy link
Member

Looks like a test or benchmark needs to be updated. I know this has grown big, if you want to just comment out the tests that are failing with a TODO to re-enable them, we can merge and file follow-up PRs to get the remaining bits. Whatever you'd find easier and more satisfying <3

@MrGunflame
Copy link
Contributor Author

Yeah, I totally forgot that nightly and benches exist. I can probably rewrite them, but I'd need some more time for that. If the PR is blocking other changes I can of course comment out the benches and add them in later again.

@seanmonstar
Copy link
Member

There's some other items that are somewhat separate, but I think they will cause merge conflicts with this, so getting it merged sooner than later will probably mean less pain. For instance, I was about to start deleting hyper::Server and hyper::server::accept, but realized that'd cause conflicts with this. But up to you, there's other things to work on too.

@MrGunflame
Copy link
Contributor Author

Alright, I will just comment them out for now I guess, might be able to add them in later in another PR or work on some other changes then.

@seanmonstar seanmonstar mentioned this pull request Jul 28, 2022
Merged
@seanmonstar
Copy link
Member

Thanks for taking this so far! I've squashed and fixed up the last couple conflicts and merged in #2929.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove the tcp cargo feature
3 participants
@MrGunflame @seanmonstar @Robert-Cunningham