zio / zio-http

A next-generation Scala framework for building scalable, correct, and efficient HTTP clients and servers
https://zio.dev/zio-http
Apache License 2.0
781 stars 392 forks source link

Bug or feature in the client? #3042

Closed pablf closed 3 weeks ago

pablf commented 1 month ago

I've seen triggered a PrematureClosedChannelException when sending a lot of requests from a client. It seems triggered only when sending multi-part forms and when the server's limit are surpassed. Here is a reproducer. @kyri-petrou any thoughts?

kyri-petrou commented 4 weeks ago

@pablf the issue here is in this line: https://github.com/pablf/client-reproducer/blob/af5b815a7dc8bdcc59a61c1c7ee9721233ed8f11/src/test/scala/SizeLimitsSpec.scala#L60

Since the server is streaming the response, if the scope is closed before the server is done streaming that forcefully interrupts the connection which results in an error. The solution here is to "await" for the server to stop streaming:

status <- ZIO.scoped {
          client(request).flatMap(v => v.ignoreBody.as(v.status))
        }

One of the things I'm unsure of however is whether we want to have this behaviour as a default. If the user decides to close the scope prior to the stream finishing, should we be raising an error? Or should we log a warning and exit successfully?

pablf commented 3 weeks ago

It makes sense thanks! I assumed wrongly that it wasn't needed to wait if only the status was to be recovered. Is reasonable to expect something like that? We could log a warning and return a response with something like a BadBody that triggers the error when read. Could this have a better performance? This could be prone to errors in the same way but the error message could be clear in that it's caused by the user trying to read the body out of the scope.

pablf commented 3 weeks ago

I've used your code but it seems like the same error persists here. I've tried also some obvious variations to no avail.

pablf commented 3 weeks ago

@kyri-petrou, I've tried now with

status <- ZIO.scoped { client(request).flatMap(v => v.ignoreBody)}.as(expected)

so that technically the request is discarded and there is the same error.

Here I've catched the exceptions and succeed with a custom status -1, send a new valid request after all the requests were sent and checked that this last request returned Ok. The exceptions get triggered but the new request works fine.

I assume from this that the server is alright as it's able to handle new requests and that this must be a client bug because it appears also when the body is discarded. Are these assumptions correct?

kyri-petrou commented 3 weeks ago

@pablf I looked a bit into this but I need to dig a bit more. At this stage I think this might be more of an issue with the server; closing the connection before it sends back the headers (or more precisely, before they're received by the client). AFAICT the client doesn't receive the headers before the connection is closed by the server.

pablf commented 3 weeks ago

I've checked more in detail the server pipeline and it seems that HttpObjectAggregator closes the connection because when constructing a body from a multi-part form it doesn't include the length. So this is actually the expected behaviour. See https://github.com/netty/netty/blob/4.1/codec-http/src/main/java/io/netty/handler/codec/http/HttpObjectAggregator.java#L248

It might be a good idea though to include the length of the multi-part form if known in the body to make this a bit better for the client.