Support AsyncFile, Path. Added PathPart and FilePart #16286

FroMage · 2021-04-06T15:10:00Z

Support for new file types and partial versions.

I need guidance for how to handle AsyncFile properly though in the case that there are writer interceptors, because our current infra doesn't support suspending in the writer if there are writer interceptors, and if we're in the IO thread I can't very well block in the writer to read the file.

We could detect a return type of AsyncFile at build time, and throw if there are writer interceptors and force the user to use @Blocking on the endpoint.

Or we start supporting @Blocking on the writer, but that sounds like a lot of work.

Any advice @stuartwdouglas ?

geoand · 2021-04-06T15:14:09Z

We could detect a return type of AsyncFile at build time, and throw if there are writer interceptors and force the user to use @Blocking on the endpoint.

I think this is a reasonable way to start out

geoand · 2021-04-06T15:16:33Z

.../java/io/quarkus/resteasy/reactive/server/runtime/ServerVertxAsyncFileMessageBodyWriter.java

+            public void handle(Void event) {
+                response.end();
+                // Not sure if I need to resume, actually
+                ctx.resume();


Likely not, since there should be no more handlers in the chain, right?

Well, there's always cleanup, no?

What cleanup? I mean if this is the last Handler in the chain, resuming should do nothing but push the context into the event loop and just return when the event actually gets processed

In the context.close we do things like call async listeners and tear down the CDI scope and stuff, no?

Ah, good point

geoand · 2021-04-06T15:17:15Z

.../java/io/quarkus/resteasy/reactive/server/runtime/ServerVertxAsyncFileMessageBodyWriter.java

+        ctx.suspend();
+        ServerHttpResponse response = context.serverResponse();
+        response.setChunked(true);
+        file.handler(buffer -> {


Don't we also need an exception handler?

Very good point.

geoand · 2021-04-06T15:20:38Z

.../main/java/org/jboss/resteasy/reactive/common/providers/serialisers/FilePartBodyHandler.java

+import javax.ws.rs.ext.MessageBodyWriter;
+import org.jboss.resteasy.reactive.FilePart;
+
+public class FilePartBodyHandler implements MessageBodyWriter<FilePart> {


Wouldn't it be best to suffix these with BodyWriter instead of BodyHandler which is rather overloaded?

I copied the other class names, but yeah, in this case they're write-only, so good point.

geoand · 2021-04-06T15:21:49Z

...java/org/jboss/resteasy/reactive/server/providers/serialisers/ServerFilePartBodyHandler.java

+
+@Provider
+@Produces("*/*")
+@Consumes("*/*")


Consumes shouldn't be needed, right?

geoand · 2021-04-07T05:59:44Z

Also, do we really need both FilePart and PathPart? I would propose we only use the latter

FroMage · 2021-04-07T09:47:45Z

Also, do we really need both FilePart and PathPart? I would propose we only use the latter

I sorta agree, but given that we support both File and Path, I think it's friendlier to provide both.

geoand · 2021-04-07T09:48:10Z

Also, do we really need both FilePart and PathPart? I would propose we only use the latter

I sorta agree, but given that we support both File and Path, I think it's friendlier to provide both.

Yeah, I don't mind either way

FroMage · 2021-04-12T15:40:23Z

We could detect a return type of AsyncFile at build time, and throw if there are writer interceptors and force the user to use @Blocking on the endpoint.

So, I thought about this a bit more, and it turns out that I don't think this type can work with writer interceptors at all, in fact.

WriterInterceptor depends on the writer using blocking IO and using OutputStream to write, which is blocking, so all the writing must be done from the worker thread.

AsyncFile does all its reading from the IO thread.

If we're on the IO thread, I can't very well use blocking IO, because it would block itself.
If we're on a worker thread, I could do all the reading from the IO thread and the writing on the worker thread, but performance is going to be horrible, so I don't see the point in supporting that.

I'm just going to disallow AsyncFile having any WriterInterceptor.

But also, I'm now wondering if our architecture really works if we have any WriterInterceptor and we're on the IO thread? Because that will force blocking IO, and potentially block the IO thread.

WDYT @stuartwdouglas @geoand ?

FroMage · 2021-04-12T15:40:40Z

Also, I fucking hate IO and in particular blocking and async IO. BOTH.

geoand · 2021-04-12T15:54:37Z

We could detect a return type of AsyncFile at build time, and throw if there are writer interceptors and force the user to use @Blocking on the endpoint.

So, I thought about this a bit more, and it turns out that I don't think this type can work with writer interceptors at all, in fact.

WriterInterceptor depends on the writer using blocking IO and using OutputStream to write, which is blocking, so all the writing must be done from the worker thread.

AsyncFile does all its reading from the IO thread.

If we're on the IO thread, I can't very well use blocking IO, because it would block itself.

If we're on a worker thread, I could do all the reading from the IO thread and the writing on the worker thread, but performance is going to be horrible, so I don't see the point in supporting that.

I'm just going to disallow AsyncFile having any WriterInterceptor.

But also, I'm now wondering if our architecture really works if we have any WriterInterceptor and we're on the IO thread? Because that will force blocking IO, and potentially block the IO thread.

+1

WDYT @stuartwdouglas @geoand ?

Yeah, I've wondered about this myself at times.

stuartwdouglas · 2021-04-12T23:18:32Z

Yea, this is complex, the blocking API's that JAX-RS provides don't work well with non-blocking.

At the moment our output stream will just fully buffer if you are on the IO thread, which works fine for small responses but is not going to work for large responses such as files.

Maybe switch to the worker thread whenever WriterInterceptors are involved, but introduce our own non-blocking version?

In general though I think for safety maybe this whole block needs to be run in a worker thread:

quarkus/independent-projects/resteasy-reactive/server/runtime/src/main/java/org/jboss/resteasy/reactive/server/core/ServerSerialisers.java

Line 201 in 76cd569

    
           if (writer.isWriteable(entity.getClass(), context.getGenericReturnType(), context.getAllAnnotations(),

stuartwdouglas · 2021-04-12T23:27:04Z

...src/main/java/io/quarkus/resteasy/reactive/server/servlet/runtime/ServletRequestContext.java

+    }
+
+    @Override
+    public boolean isWriteQueueFull() {


isWriteQueueFull and addDrainHandler don't really make sense here. The interface really needs some Javadoc but at the moment write() is assuming you can write once, then you need to wait for a notification.

Well, I honestly didn't know how to implement those. Any hint? I did need them for the vertx async part.

stuartwdouglas · 2021-04-12T23:31:28Z

...src/main/java/io/quarkus/resteasy/reactive/server/servlet/runtime/ServletRequestContext.java

+
+    @Override
+    public ServerHttpResponse sendFile(String path, long offset, long length) {
+        context.response().sendFile(path, offset, length);


This bypasses a lot of Servlet stuff, it really needs to write the file using the Servlet API's.

Are there any specific APIs for servlet to write files?

.../java/io/quarkus/resteasy/reactive/server/runtime/ServerVertxAsyncFileMessageBodyWriter.java

.../main/java/org/jboss/resteasy/reactive/common/providers/serialisers/PathPartBodyHandler.java

...c/main/java/io/quarkus/resteasy/reactive/common/runtime/VertxAsyncFileMessageBodyWriter.java

FroMage · 2021-04-13T08:27:55Z

Maybe switch to the worker thread whenever WriterInterceptors are involved, but introduce our own non-blocking version?

Well, yeah, perhaps. I had to do that for Async-IO in RESTEasy Classic. I was hoping not to have to do it again, but…

quarkus-bot · 2021-04-13T17:41:07Z

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building `6386fea`

Status	Name	Step	Test failures	Logs	Raw logs
✖	JVM Tests - JDK 11	`Build`	Test failures	Logs	Raw logs
✖	JVM Tests - JDK 11 Windows	`Build`	Test failures	Logs	Raw logs
✖	JVM Tests - JDK 15	`Build`	Test failures	Logs	Raw logs
✖	JVM Tests - JDK 8	`Build`	⚠️ Check →	Logs	Raw logs
✖	MicroProfile TCKs Tests	`Run RESTEasy Reactive TCK`	Test failures	Logs	Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ JVM Tests - JDK 11 #

📦 integration-tests/kafka

✖ io.quarkus.it.kafka.SaslKafkaConsumerTest.testReception line 48 - More details - Source on GitHub

✖ io.quarkus.it.kafka.SslKafkaConsumerTest.testReception line 56 - More details - Source on GitHub

⚙️ JVM Tests - JDK 11 Windows #

📦 extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment

✖ io.quarkus.resteasy.reactive.server.test.providers.FileTestCase.testFiles line 51 - More details - Source on GitHub

⚙️ JVM Tests - JDK 15 #

📦 integration-tests/kafka

✖ io.quarkus.it.kafka.SaslKafkaConsumerTest.testReception line 48 - More details - Source on GitHub

✖ io.quarkus.it.kafka.SslKafkaConsumerTest.testReception line 56 - More details - Source on GitHub

⚙️ MicroProfile TCKs Tests #

📦 resteasy-reactive-testsuite/tests

✖ com.sun.ts.tests.jaxrs.spec.filter.interceptor.JAXRSClient0021.fileWriterClientInterceptorTest line 591 - More details - Source on GitHub

FroMage · 2021-04-14T09:11:15Z

OK, the kafka failures are not mine. But this Windows failure is very interesting:

Expected header "Content-Length" was not "448", was "453". Headers are:
content-length=453

So the file size is different on windows? That's… how do we explain that?

geoand · 2021-04-14T09:14:41Z

OK, the kafka failures are not mine.

If you rebase onto the current main, then those will likely go away

geoand · 2021-04-14T09:15:04Z

So the file size is different on windows? That's… how do we explain that?

This is probably stupid, but line endings maybe?

FroMage · 2021-04-14T09:20:07Z

This is probably stupid, but line endings maybe?

Well, but who would remove/add them?

FroMage · 2021-04-14T09:27:56Z

The TCK failure is also interesting:

It's a WriterInterceptor that hijacks the OutputStream where we write the File, and replaces the contents with other stuff. Meanwhile I added the Content-Length header in the File writer, because we know it and it's good form. But it doesn't work if the output isn't actually what we produce.

This looks like a silly test: it should probably remove the Content-Length header, no?

Or should I drop the Content-Length when returning a File? That seems a bit wrong, no? TCK or not, I think it's the interceptor's fault for making a mess and it should clean up, no?

geoand · 2021-04-14T09:31:05Z

This is probably stupid, but line endings maybe?

Well, but who would remove/add them?

Well, the server reads the file from the filesystem, right? So in that case it would likely add the Windows line ending when sending the file back, while the test is looking for the string length having the string memory, no?

geoand · 2021-04-14T09:32:43Z

The TCK failure is also interesting:

It's a WriterInterceptor that hijacks the OutputStream where we write the File, and replaces the contents with other stuff. Meanwhile I added the Content-Length header in the File writer, because we know it and it's good form. But it doesn't work if the output isn't actually what we produce.

This looks like a silly test: it should probably remove the Content-Length header, no?

Yeah, I would expect that.

Or should I drop the Content-Length when returning a File? That seems a bit wrong, no? TCK or not, I think it's the interceptor's fault for making a mess and it should clean up, no?

I think you have done the right thing and the test is making a mess

geoand · 2021-04-14T09:33:06Z

This is probably stupid, but line endings maybe?

Well, but who would remove/add them?

Well, the server reads the file from the filesystem, right? So in that case it would likely add the Windows line ending when sending the file back, while the test is looking for the string length having the string memory, no?

Also, are there 5 lines in the result? If not, we can simply ignore my comment :)

FroMage · 2021-04-14T09:52:36Z

Also, are there 5 lines in the result? If not, we can simply ignore my comment :)

There are. But I don't get it. I'm expecting a file of size 448, which is the size of the String I'm comparing it to.
The server reads it as bytes, it should never interpret any line endings while reading or writing. I'm not even looking at encoding or anything. And I get back from the server 5 bytes too many. Because it set that header, so the file system must have reported it as 5 bytes more.

This isn't even about encoding or something adding line endings. I don't think the FS on Windows goes and reports file sizes with adjusted line endings.
Unless, the checked-out lorem.txt file itself has been adjusted. Huh, I guess that's possible. Perhaps git does that, or some other process we have in place.

Because it changes the content, and the server is right in adding this header. See quarkusio/quarkus#16286

FroMage · 2021-04-14T10:00:19Z

OK, adjusted for Windows, pushed new TCK, rebased on main.

quarkus-bot · 2021-04-14T11:52:41Z

Failing Jobs - Building `8151c96`

Status	Name	Step	Test failures	Logs	Raw logs
✖	Gradle Tests - JDK 11 Windows	`Verify dependencies`	Test failures	Logs	Raw logs

Full information is available in the Build summary check run.

Test Failures

⚙️ Gradle Tests - JDK 11 Windows #

📦 integration-tests/gradle

✖ io.quarkus.gradle.devmode.QuarkusDevDependencyDevModeTest.main() line 14 - More details - Source on GitHub

FroMage · 2021-04-14T12:26:46Z

I don't see how that gradle/windows/devmode failure can be related, so restarting.

geoand · 2021-04-14T12:41:42Z

It's probably just a flake

FroMage · 2021-04-14T14:17:34Z

CI actually passed, but the Build summary failed. What do we do in these cases @gsmet ?

gsmet · 2021-04-19T14:58:22Z

Ah I know, the outdated status was probably not hidden properly. I'll take this case into account. You can safely merge this if all is good for you all.

geoand · 2021-04-19T15:02:25Z

@FroMage I assume this is ready to go?

cescoffier · 2021-04-19T15:06:38Z

Merged!

cescoffier · 2021-04-19T15:06:56Z

(@FroMage told me it was ok)

geoand · 2021-04-19T15:07:09Z

👍🏼

FroMage · 2021-04-19T15:24:04Z

Famous Last Words™

Thanks ;)

Because it changes the content, and the server is right in adding this header. See quarkusio/quarkus#16286

FroMage added the area/resteasy-reactive label Apr 6, 2021

FroMage requested review from stuartwdouglas and geoand April 6, 2021 15:10

quarkus-bot bot added the area/documentation label Apr 6, 2021

geoand reviewed Apr 6, 2021

View reviewed changes

stuartwdouglas requested changes Apr 12, 2021

View reviewed changes

FroMage force-pushed the 15891 branch 2 times, most recently from 3c6c576 to 6386fea Compare April 13, 2021 15:32

FroMage mentioned this pull request Apr 13, 2021

AsyncFile: allow access to file size eclipse-vertx/vert.x#3892

Closed

FroMage marked this pull request as ready for review April 13, 2021 15:35

FroMage added a commit to quarkusio/resteasy-reactive-testsuite that referenced this pull request Apr 14, 2021

Adapt TCK WriterInterceptor to remove the Content-Length header

531734d

Because it changes the content, and the server is right in adding this header. See quarkusio/quarkus#16286

FroMage added 2 commits April 14, 2021 11:59

Support AsyncFile, Path. Added PathPart and FilePart

3dc647f

RESTEasyReactive: workaround for Netty upgrade

8151c96

FroMage force-pushed the 15891 branch from 6386fea to 8151c96 Compare April 14, 2021 10:00

geoand requested a review from stuartwdouglas April 19, 2021 15:01

cescoffier merged commit 086c700 into quarkusio:main Apr 19, 2021

quarkus-bot bot added this to the 2.0 - main milestone Apr 19, 2021

geoand mentioned this pull request Apr 20, 2021

Add streaming support for files in RESTEasy Reactive #15891

Closed

FroMage linked an issue Apr 20, 2021 that may be closed by this pull request

Add streaming support for files in RESTEasy Reactive #15891

Closed

FroMage added a commit to quarkusio/resteasy-reactive-testsuite that referenced this pull request Apr 20, 2021

Adapt TCK WriterInterceptor to remove the Content-Length header

7ed2bfb

Because it changes the content, and the server is right in adding this header. See quarkusio/quarkus#16286

Support AsyncFile, Path. Added PathPart and FilePart #16286

Support AsyncFile, Path. Added PathPart and FilePart #16286

Conversation

FroMage commented Apr 6, 2021

geoand commented Apr 6, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geoand commented Apr 7, 2021

FroMage commented Apr 7, 2021

geoand commented Apr 7, 2021

FroMage commented Apr 12, 2021

FroMage commented Apr 12, 2021

geoand commented Apr 12, 2021

stuartwdouglas commented Apr 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FroMage commented Apr 13, 2021

quarkus-bot bot commented Apr 13, 2021 • edited

Failing Jobs - Building 6386fea

Test Failures

⚙️ JVM Tests - JDK 11 #

📦 integration-tests/kafka

⚙️ JVM Tests - JDK 11 Windows #

📦 extensions/resteasy-reactive/quarkus-resteasy-reactive/deployment

⚙️ JVM Tests - JDK 15 #

📦 integration-tests/kafka

⚙️ MicroProfile TCKs Tests #

📦 resteasy-reactive-testsuite/tests

FroMage commented Apr 14, 2021 • edited

geoand commented Apr 14, 2021

geoand commented Apr 14, 2021

FroMage commented Apr 14, 2021

FroMage commented Apr 14, 2021

geoand commented Apr 14, 2021

geoand commented Apr 14, 2021

geoand commented Apr 14, 2021

FroMage commented Apr 14, 2021

FroMage commented Apr 14, 2021

quarkus-bot bot commented Apr 14, 2021

Failing Jobs - Building 8151c96

Test Failures

⚙️ Gradle Tests - JDK 11 Windows #

📦 integration-tests/gradle

FroMage commented Apr 14, 2021

geoand commented Apr 14, 2021

FroMage commented Apr 14, 2021

gsmet commented Apr 19, 2021

geoand commented Apr 19, 2021

cescoffier commented Apr 19, 2021

cescoffier commented Apr 19, 2021

geoand commented Apr 19, 2021

FroMage commented Apr 19, 2021

quarkus-bot bot commented Apr 13, 2021 •

edited

Failing Jobs - Building `6386fea`

FroMage commented Apr 14, 2021 •

edited

Failing Jobs - Building `8151c96`