dgrr / http2

HTTP/2 implementation for fasthttp
Apache License 2.0
208 stars 36 forks source link

Post with empty data hangs #15

Closed AMekss closed 3 years ago

AMekss commented 3 years ago

Hey, thanks for your effort in making this. In order to help I'm trying to understand what's the state of this project in terms of compliance to the H2 spec. So I'm trying to run h2spec suite against simple server. Unfortunately it hangs during request initialization on this line for client and on this line for server. I might be wrong on this however it looks like that both server and client are waiting on TCP socket for data to come in. While I was fiddling around I discovered that I can repeat this behaviour if I send a POST request with no data:

curl -v -k -XPOST "https://localhost:8443/"
*   Trying ::1...
* TCP_NODELAY set
* Connection failed
* connect to ::1 port 8443 failed: Connection refused
*   Trying 127.0.0.1...
* TCP_NODELAY set
* Connected to localhost (127.0.0.1) port 8443 (#0)
* ALPN, offering h2
* ALPN, offering http/1.1
* successfully set certificate verify locations:
*   CAfile: /etc/ssl/cert.pem
  CApath: none
* TLSv1.2 (OUT), TLS handshake, Client hello (1):
* TLSv1.2 (IN), TLS handshake, Server hello (2):
* TLSv1.2 (IN), TLS handshake, Certificate (11):
* TLSv1.2 (IN), TLS handshake, Server key exchange (12):
* TLSv1.2 (IN), TLS handshake, Server finished (14):
* TLSv1.2 (OUT), TLS handshake, Client key exchange (16):
* TLSv1.2 (OUT), TLS change cipher, Change cipher spec (1):
* TLSv1.2 (OUT), TLS handshake, Finished (20):
* TLSv1.2 (IN), TLS change cipher, Change cipher spec (1):
* TLSv1.2 (IN), TLS handshake, Finished (20):
* SSL connection using TLSv1.2 / ECDHE-RSA-AES128-GCM-SHA256
* ALPN, server accepted to use h2
* Server certificate:
*  subject: O=fasthttp test
*  start date: Feb 17 21:19:20 2021 GMT
*  expire date: Feb 17 21:19:20 2022 GMT
*  issuer: O=fasthttp test
*  SSL certificate verify result: self signed certificate (18), continuing anyway.
* Using HTTP2, server supports multi-use
* Connection state changed (HTTP/2 confirmed)
* Copying HTTP/2 data in stream buffer to connection buffer after upgrade: len=0
* Using Stream ID: 1 (easy handle 0x7fc47980ba00)
> POST / HTTP/2
> Host: localhost:8443
> User-Agent: curl/7.64.1
> Accept: */*
>
* Connection state changed (MAX_CONCURRENT_STREAMS == 100)!
^C <= ^^^ it hangs here so I had to kill the process with ctrl+c

For the record I also setup a simple HTTP/2 server from the stdlib for the reference and it mostly conform against h2spec (146 tests, 142 passed, 0 skipped, 4 failed)

Anyways I think h2spec could provide a good target and progress meter for this project. I'll try to figure out how to run it against this project, however if you have any clue where to look or how to fix that it would be great:)

dgrr commented 3 years ago

Hello,

Thanks for doing some testing. The problem is here. I should change it to:

else if !strm.hfr.EndStream() {
  ... = stateAwaitData
}
...

Currently I am working on the client side. When I am done with a basic client, I'll come back to this.

Thanks for your contribution. Testing is very important.

AMekss commented 3 years ago

Yeah I just fond that line too :) anyways thanks for pointing me into a right direction. I'll try to patch it and see whether h2spec runs.

I'll also try to read http2 specs some more and try to figure out what would be a better way to decide when to execute handler and when await for data. I'm not sure yet about HTTP/2 however in HTTP/1.1 it is possible to send a data payload within GET request too. Maybe read Content-Length/Transfer-Encoding header fields and base decision on that? wdyt?

dgrr commented 3 years ago

Good. No... In HTTP/2 you know you read the whole body when you get a DataFrame with the flag EndStream on. That's on the section 5.1, I think, on the stream states. I think it should be possible to send the payload in a GET request. I don't know if the spec specifies that, but I don't care as long as servers accept it.

The whole idea in HTTP/2 I think is just replacing the word connection with stream in HTTP/1.1. So, you don't open a connection for every request, you open a new stream. Thus, we'll need to simulate that behavior. I am already doing that on the client. I didn't push yet. I'll do this night, when I have something finished.

AMekss commented 3 years ago

I figured all the missing peaces to run h2spec against simple server. A bit later today I'll publish a PR with fixes so we can discuss it whether they make sense to be merged.

The current state is as follows: 146 tests, 45 passed, 0 skipped, 101 failed the full report is here (didn't want to spam this issue)