[WIP] netty-cats: support multipart requests #3933
Conversation
| monad | ||
| .blocking { | ||
| // this operation is the one that does potential I/O (writing files) | ||
| // TODO not thread-safe? (visibility of internal state changes?) |
There was a problem hiding this comment.
question: This is the part that concerns me. The decoder is stateful, so it has an internal state changed with every call to offer. Each time a new HttpContent flows through the stream, the monad.blocking call gets a thread from the blocking pool, where it calls offer. There are no race conditions, but aren't internals of the decoder risking visibility issues because of such circumstances?
There was a problem hiding this comment.
This is fine as long as there's some memory barrier along the way (see e.g. https://stackoverflow.com/questions/12438464/volatile-variables-and-other-variables). There should be a couple of those along the way, I suspect putting back/obtaining a thread to execute blocking ops itself imposes at least one such barrier.
| file <- createFile(serverRequest) | ||
| _ <- writeBytesToFile(httpData.get(), file) | ||
| } yield file) | ||
| else monad.unit(httpData.getFile()) |
There was a problem hiding this comment.
note: Netty decoder creates the file (if it's > 16KB), so we can just get its handle here.
|
@kciesielski @adamw Is this PR still planned to be worked on? I am currently migrating our codebase to use Netty Sync as interpreter, but found out that multipart is not supported. I was thinking of trying to make a PR for multipart support with Netty Sync. In that case, should I base my PR on top of this one, or should I use this PR as inspiration instead (e.g., the abstract method |
|
@amatho Yes, definitely! I think there were some problems with the playframework library code, and we considered pulling fragments into our codebase and modifying. But we're lacking time, so any help would be greatly appreciated. I think you can either continue this PR, or just create a new one reusing bits of code from here where appropriate. |
|
Perfect, I'll see what I can do! |
This PR is based on #3933 and implements support for multipart requests/responses with Netty. Currently only for Netty Sync, I will try to implement for Cats, ZIO, etc. in another PR. Adds a dependency on the `httpmime` library for using the `MultipartEntityBuilder` (as used in JDK HTTP server), since Netty unfortunately has no built-in way to make multipart response bodies. (I have not included the original commits from #3933, but if that's desired I can do some rebasing to include them!)
|
Superseded by #4849, which implements multipart for netty-sync. Parts of this code might still be needed for a netty-cats implementation. |
|
The issue for the other multipart implementations is: #4851, vote if you need multipart in netty-cats (or -future, or -zio) |
Partially addresses #499 - adds support for multipart requests. Responses will be implemented in a separate PR.
NettyServerHandler, a multipart request is just aStreamedHttpRequestpassed toNettyRequestBodyNettyRequestBodydelegates processing of such a request to backend-specific implementation ofpublisherToMultipartpublisherToMultipartprocesses converts the reactive Publisher to a fs2 Stream of elements of typeHttpContent. Then these elements are passed to a statefulio.netty.handler.codec.http.multipart.HttpPostRequestDecoder, which, after collecting enough of them, can produce full parts.NettyRequestBody.toRawPart- intended to be common for all backends.