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

Shouldn't pre-computed HTTP headers trigger "Stripped" classification? #8205

Open
synopse opened this issue May 10, 2023 · 7 comments
Open

Comments

@synopse
Copy link

synopse commented May 10, 2023

Reading general requirement number 2 :

This project intends to measure performance of production-grade deployments of real-world web application frameworks and platforms. All implementations are expected (but not required) to be based on robust implementations of the HTTP protocol. Implementations that are not based on a realistic HTTP implementation will be marked as Stripped. Rather than stipulate a rigid specification of HTTP requirements, we defer to common sense: the implementation should provide a viable subset of HTTP to reasonably unbiased outsiders familiar with HTTP.

and for instance requirement iv of the plain text requirement :

However, the rest of the response must be fully composed on the spot.

Some frameworks do not compute the HTTP headers at all, but return a fixed value, including HTTP/1.1 200 OK and the Content-Length: integer value, just appending the current date to this hardcoded buffer. This is not a realistic HTTP implementation to my (common) sense. There is no point of allowing to pre-compute the headers but disallowing to pre-compute the response body - the former clearly depending on the later content.

Such patterns appear for instance in this plaintext or this json implementations. My guess is that such implementations should be marked as Stripped, or fixed to use a HTTP headers generation library, as any realistic production-grade framework would do.

The main issue with such fixed headers is that the server is not compatible with HTTP/1.0 requests or Connection: Close headers, which are part of the HTTP protocol, and should be properly implemented on the server side.

Perhaps there are other implementations with the same dubious pattern.

@synopse
Copy link
Author

synopse commented May 10, 2023

After a quick search, other similar findings - some clearly breaking the requirements:
https://github.com/TechEmpower/FrameworkBenchmarks/blob/master/frameworks/Go/gnet/src/main.go#L29
https://github.com/TechEmpower/FrameworkBenchmarks/blob/master/frameworks/Rust/actix/src/main_server.rs#L23
https://github.com/TechEmpower/FrameworkBenchmarks/blob/master/frameworks/Rust/faf/src/main.rs
https://github.com/TechEmpower/FrameworkBenchmarks/blob/master/frameworks/Rust/ntex/src/main_plt.rs

IMHO a simple way to validate the HTTP implementation could be to emit a simple curl request with a Connection: close header (or sent as HTTP/1.0), which should be properly parsed on the server, then either Connection: close or HTTP/1.0 200 OK or Closing connection should be part (grep) of the response, and the socket closed:

~/techempower-bench$ curl --http1.0 -v localhost:8080/plaintext
*   Trying 127.0.0.1:8080...
* Connected to localhost (127.0.0.1) port 8080 (#0)
> GET /plaintext HTTP/1.0
> Host: localhost:8080
> User-Agent: curl/7.88.1
> Accept: */*
> 
* HTTP 1.0, assume close after body
< HTTP/1.0 200 OK
< Server: M
< Date: Wed, 10 May 2023 15:01:35 GMT
< Content-Length: 13
< Content-Type: text/plain
< Connection: Close
< 
* Closing connection 0

or a simple
curl -H "Connection: Close" -v localhost:8080/plaintext

This test should be run on all endpoints which are marked as "approach": "Realistic" in their benchmark_config.json file - i.e. not only /plaintext but also all other /db /json /updates and /fortunes.

From a wild guess of the source code I have seen, quite a few frameworks would then appear as Stripped and should be fixed/implemented in order to be part of the official ranking.

@fakeshadow
Copy link
Contributor

just want to point out actix marks the bench you linked as stripped already. they really should just remove the code entirely but I was told it's there for future optimization despite I have seen zero effort they put in it for years. but still.

@joanhey
Copy link
Contributor

joanhey commented May 10, 2023

The benchmark with the time is bigger, and we need to help.

We can hep in the verify stage, to add more small details to have a fair play.

And others we can't verify, like the Content-Length:, that for me need to be calculated and not hardcoded, is not verifiable.
But will be enough to add it to the requirements, and we will mark the frameworks that bypass the requirements.
It's easy to add in the requirements: it's forbidden to hard code the Content-Length:.

@franz1981
Copy link
Contributor

Related partially to this: #7984

but on request parsing side: if parsing won't happen, no http headers are materialized/evaluated to drive HTTP server logic as well

@synopse
Copy link
Author

synopse commented Sep 4, 2023

Another framework with pre-computed headers for /plaintext - which won't respond to basic "Connection: close" request so should be marked as "stripped":

private static final CharSequence HELLO_WORLD_LENGTH = HttpHeaders.createOptimized("" + HELLO_WORLD.length());

@synopse
Copy link
Author

synopse commented Sep 4, 2023

Related partially to this: #7984

but on request parsing side: if parsing won't happen, no http headers are materialized/evaluated to drive HTTP server logic as well

You are right, this is the same issue.
IMHO we could sum both issue as: "If the server doesn't properly respond to Connection: Close, then it should be marked as Stripped". This is easy to check, and a fair implication of the TFB requirement #2 of a "viable implementation of HTTP".

@franz1981
Copy link
Contributor

franz1981 commented Sep 7, 2023

@synopse as explained in another comment, Vertx doesn't precompute headers, but just the ASCII string objects (which will still be encoded as usual) in the case-insensitive format, with case insensitive hash code compute once: it's not related the encoding of headers, which will happen, but a way to reduce the cost while putting them in the case-insensitive header map which will be encoded later and it's strongly suggested to happen in production too.

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

No branches or pull requests

4 participants