-
Notifications
You must be signed in to change notification settings - Fork 20
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
HTTP body not read to completion. Dropping connection. #714
Comments
Thanks for opening the issue! It looks like this is specific to the jdk-http-client? We should transfer to the issue to its dedicated repository. cc @amesgen |
@armanbilge Indeed, I can confirm, that the issue disappears when using OkHttpClient. Should I re-post the issue in https://github.com/http4s/http4s-jdk-http-client or perhaps @amesgen will do the transfer? |
Thanks for the report, I can reproduce this. This should definitely be fixed, but it should always be possible to work around this by consuming the response body, right? |
Hm, probably yes, but one should not expect such a behavior from the user, what do you think? For what it is worth, I first encountered the error via sttp's synchronous HttpClientSyncBackend, which may suggest the problem might persist even when using a raw Java HttpClient. |
I think Embers solution may be generic enough to be leveraged here. It keeps track and drains if it wasn't normally so the connection can be preserved. |
The error persists even when using a vanilla Java client: object HttpConnectionCloseBodyDrain extends IOApp.Simple {
def run: IO[Unit] = {
val routes = HttpRoutes.of[IO] {
case POST -> Root / "test" / "post" =>
Ok()
}
val javaClient = HttpClient.newBuilder().version(Version.HTTP_1_1).build()
val req = HttpRequest
.newBuilder(new URI("http://localhost:8080/test/post?param=XXX)"))
.POST(HttpRequest.BodyPublishers.ofString("dummy body"))
.build()
BlazeServerBuilder[IO]
.bindHttp(8080, "localhost")
.withHttpApp(Router("/" -> routes).orNotFound)
.resource
.use { _ =>
IO {
for (i <- 0 until 100) {
val response = javaClient.send(req, BodyHandlers.ofString())
println(s"$i: $response")
}
}
}
}
}
|
Hi folks, hope you're good. Any updates on this? I see this error in the scala-steward we operated internally. When it tries to open a PR, it sees the following exception:
I'd be happy to fix this if I can get some pointers. This issue tells me a bit about the issue, but I guess some people already tried to fix it to no avail. Maybe there is more information available at this point in time. |
I'm not aware of anyone who has worked on it. Chris's suggestion seems good. Also we can fix this in Steward by draining the response bodies. |
Looking into implementing http4s/http4s#4348 here I believe |
I'm not sure if I'll be able to pull it off. |
Feel free to open a draft PR with whatever you have! Even if you are no longer able to work on it, maybe someone else can pick it up. |
@armanbilge#6192 wrt to #714 I think the reason I'm not able to fix the issue is that I don't understand the issue properly. I don't understand how this fix in Ember client makes it work. We send a bunch of requests to a server. These requests have a body, and we use http 1.1 so we're using The issue arise when the server sends an early response: the server responds before the client is finished sending the request (this can happen when you don't drain the request body (as we do in this example)). If I look at the PR, it looks like in a typical exchange, the ember client does this:
W/o going into more details here (will pick that up later), I'm unable to understand how 2) can affect the behaviour of a client interacting with a server that does not drain the request body. It should just parse body as they are coming in on the wire. As such, I'm not sure I understand how the JDK can experience an issue in this situation:
Why does it care if the server has not read the bytes it was suppose to read? I think it's my understanding of the lower protocols that's wrong or insufficient. While playing around, I found two issues with Chunked encoding (I was trying to slow down the request writing process - akin to a large payload, so I tried using a https://github.com/daddykotex/scala-scripts/tree/e1ef47f5a3bcae779f4aa879e8f0f6dcb7564637/http4s |
Hmm, things seem to be a bit muddied 🤔 and apologies if I haven't looked closely enough at this issue.
My understanding has been that this issue is not related to the server. This issue is related to the fact that the response body sent from the server to the client must be drained on the client. That's why I opened the steward PR adding those drains.
In general, undrained bodies (both request and response) pose a problem. Here's why: ideally a single connection (i.e. a TCP socket) is reused for multiple request/response pairs. If a request/response is not read in its entirety, then its bytes are still sitting in that socket. So if you attempt to send or read new data on that socket (i.e. for the next request/response pair), it will be mixed/corrupted with these undrained bytes, and that poses a problem. |
No need to apologies, it's already kind enough of you to support me
Ok so my understanding is not so messed up after all! At first, after reading the issue description, I thought the problem happened when the server was not draining the issue (so I focused on that, and I still can't understand how that would affect the client). But now that you say that the problem happens because the client is not draining, I tried it and still got an error. The following snippet, still cause the issue: //> using scala "2.13.10"
//> using lib "org.http4s::http4s-dsl:0.23.19-RC1"
//> using lib "org.http4s::http4s-jdk-http-client:0.9.0"
//> using lib "org.http4s::http4s-ember-client:0.23.19-RC1"
//> using lib "org.http4s::http4s-ember-server:0.23.19-RC1"
import cats.effect._
import cats.implicits._
import com.comcast.ip4s._
import org.http4s.client.dsl.Http4sClientDsl
import org.http4s.dsl.io._
import org.http4s.ember.server._
import org.http4s.implicits._
import org.http4s.jdkhttpclient.JdkHttpClient
import org.http4s.{HttpRoutes, Uri}
import java.net.http.HttpClient
import java.net.http.HttpClient.Version
// This is a *manual* test for the body leak fixed in #714
object HttpConnectionCloseBodyDrain extends IOApp.Simple {
def run: IO[Unit] = {
val routes = HttpRoutes.of[IO] { case POST -> Root / "test" / "post" =>
Ok()
}
val client = JdkHttpClient[IO](
HttpClient.newBuilder().version(Version.HTTP_1_1).build()
)
val postRequests = {
val clientDsl = Http4sClientDsl[IO]
import clientDsl._
val req = POST(
"dummy body",
Uri.unsafeFromString("http://localhost:8080/test/post?param=XXX)")
)
val reqResource = client.run(req)
List.from(0 until 100).traverse_ { i =>
reqResource.use { response =>
response.body.compile.drain *>
IO.println(s"$i: $response")
}
}
}
EmberServerBuilder
.default[IO]
.withHost(ipv4"127.0.0.1")
.withPort(port"8080")
.withHttpApp(routes.orNotFound)
.build
.use(_ => postRequests)
}
}
Side note: can you tell me if what I'm picturing is wrong? Given a tcp connection involving TCP endpoint A and TCP endpoint B, is fair to say that, from the point of view of one of them, A for example, you basically have access to two things:
And that
|
Aha, thanks for this! That is very helpful. Indeed, in this reproducer, the problem is almost definitely that the server is not draining the request bytes. Does it fix if you do that? (Also, I realize that this may be what the original reproducer in the issue may have been showing, if that's the case, sorry 😅.)
Did my explanation above not answer your questions? Please let me know how I can clarify it :)
Well, it does matter, for two reasons.
|
Ah okk, that'd make sense. For 2), if that happens the error happens on the B side, right? Thinking it can read a new request, it just reads the leftover from the last one. But if we're seeing client side error related to parsing headers so I would also think it's related to the response handling on the client side |
Exactly! So in (2), on the B side there is an error. So, instead of responding, it closes the connection. Thus, on the A side you get |
A batch of POST requests cause connection to be dropped due to unconsumed request body:
HTTP body not read to completion. Dropping connection.
Full log:
And this - once in a while - makes client request lost on the wire.
For this to happen:
Checked with blaze and ember servers.
The text was updated successfully, but these errors were encountered: