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

Allow HTTP/0.9 responses behind a flag (fixes #2468) #2473

Merged
merged 2 commits into from
Mar 26, 2021

Conversation

nox
Copy link
Contributor

@nox nox commented Mar 19, 2021

The two first commits work around some issues I had with VS Code and rust-analyzer.

@nox
Copy link
Contributor Author

nox commented Mar 19, 2021

I guess someone forgot to run tests and that someone was me.

@nox
Copy link
Contributor Author

nox commented Mar 19, 2021

@seanmonstar Please tell me which kind of test you would want and in which file.

@nox
Copy link
Contributor Author

nox commented Mar 20, 2021

I've filed an issue against rust-analyzer for that macro_rules indirection problem.

@nox
Copy link
Contributor Author

nox commented Mar 22, 2021

I have removed the rust-analyzer macro_rules work around as the bug has been fixed upstream.

@nox
Copy link
Contributor Author

nox commented Mar 23, 2021

We probably want this option to apply only to initial HTTP/1 requests and not those happening after the first one on a keep-alive connection.

@seanmonstar
Copy link
Member

Nice work, thanks!

Please tell me which kind of test you would want and in which file.

The tests in hyper are a little messy, but for this, there's probably 2 relevant places: in proto/h1/role.rs, perhaps a couple simple decode tests that it works. A test in tests/client.rs (excuse the mess) would be useful to make sure the rest of the stuff is hooked up properly (basically the config, and that you got an http::Response with the correct Version returned and the body).

@nox
Copy link
Contributor Author

nox commented Mar 24, 2021

In addition to probably not wanting to interpret 0.9 responses on non-initial requests what do we do about the HTTP/2 parsing fallback too? Never mind I had a brain fart.

@nox
Copy link
Contributor Author

nox commented Mar 24, 2021

I added 3 tests and prevented accepting an HTTP/0.9 response after the first one we receive (though I didn't write a test for that but I will).

@seanmonstar
Copy link
Member

Just checking, this has everything you meant to include? I don't see anything missing, but you mentioned a few questions that I see are now struck out, so just making sure.

@nox
Copy link
Contributor Author

nox commented Mar 25, 2021

Yeah, the struck-out thought is that we don't need to care about h2 fallback given this is about responses, a detail I forgot when I wrote that :)

As for the rest, it feels complete to me, but maybe we should have a test to check against HTTP/0.9 responses on a keep-alive connection after the first response? You decide.

@seanmonstar
Copy link
Member

maybe we should have a test to check against HTTP/0.9 responses on a keep-alive connection after the first response?

Hm, so, if we're reusing an HTTP/1.x connection, would it ever make sense for the server to have downgraded to HTTP/0.9? I want to say that the first response kind of solidifies the version of the whole connection, but maybe I'm wrong. If we assume it cannot downgrade, then that'd mean that any server that, after sending an HTTP/1.x response, decides to just start sending jibberish, we would assume it's busted...

You know, thinking more, probably it's worth to enforce that, since I think a more likely scenario is a server accidentally sending a bigger body than it declared, and we would want to say it was an error, not another response in HTTP/0.9. Whatcha think?

@nox
Copy link
Contributor Author

nox commented Mar 25, 2021

I think my code already refuses to ever downgrade to HTTP/0.9 after we received a first response, so I think we agree we should enforce that, yes, hah.

Are there tests for keep-alive stuff already that I can take inspiration from?

@seanmonstar
Copy link
Member

Are there tests for keep-alive stuff already that I can take inspiration from?

There are a few tests for keep-alive, but I think the way they are written is pretty bad. I've wanted to re-do hyper's test suite for a while now (it was some of the first Rust code I ever wrote, and instead of improving it, I've just adding junk on top). I'd probably rewrite those using tokio-test, which allows us to define an IO type and what expected reads and writes are.

I'm fine merging this without that specific test, to get it released sooner, unless you'd prefer I hold off.

@seanmonstar seanmonstar merged commit 68d4e4a into hyperium:master Mar 26, 2021
BenxiangGe pushed a commit to BenxiangGe/hyper that referenced this pull request Jul 26, 2021
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.

2 participants