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.

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

Fix hyper #2611

Closed
wants to merge 1 commit into from
Closed

Fix hyper #2611

wants to merge 1 commit into from

Conversation

polachok
Copy link
Contributor

Upgrades hyper to latest master

@mention-bot
Copy link

Thanks @polachok for contributing to The Framework Benchmarks! @steveklabnik, code you've worked on has been modified. If you have the chance, please review. If you wish to unsubscribe from these notices, please add your name to the userBlacklist array in the .mention-bot file.

Copy link
Contributor

@steveklabnik steveklabnik left a comment

Choose a reason for hiding this comment

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

one extremely tiny nit, but LGTM

@@ -33,8 +32,9 @@ impl Service for TechEmpower {
Response::new()
.with_header(ContentLength(HELLOWORLD.len() as u64))
.with_header(ContentType(Mime(TopLevel::Text, SubLevel::Plain, vec![(Attr::Charset, Value::Utf8)])))
.with_header(header::Server("hyper/async".to_owned()))
Copy link
Contributor

Choose a reason for hiding this comment

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

standard rust style is to_string not to_owned (but they do the exact same thing)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@alexcrichton
Copy link
Contributor

Oh one strategy this may wish to take as well is to check in Cargo.lock and avoid the need for rev = "..." in the dependency specification, but that's completely optional!

@steveklabnik
Copy link
Contributor

@alexcrichton I don't remember why I didn't do it that way, but I think there was some kind of issue. Can't remember; that was like two years ago or something. I could have messed it up too.

@alexcrichton
Copy link
Contributor

Ah yeah tokio-minihttp nowdays has Cargo.lock, so I figured it'd be ok to throw in here as well (but again, just an optional extension)

@arthurprs
Copy link

It's a bit naive to allocate a string on every reques just to set the server header. I'm sure it doesn't matter in practice but it may show in a hello-world benchmark

@polachok
Copy link
Contributor Author

@arthurprs well, that's how Server is defined https://hyper.rs/hyper/0.8.0/hyper/header/struct.Server.html

@arthurprs
Copy link

I see, Server is also required by the test rules.

@NateBrady23
Copy link
Member

Looks like there's agreement to close this in favor of #2619

Closing this and merging that. Thanks guys!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants