Refactor ResteasyReactiveOutputStream so that it can be used by Quarkus CXF #40995

ppalaga · 2024-06-05T17:58:02Z

This PR is motivated by Quarkus CXF's need to have a class similar to org.jboss.resteasy.reactive.server.vertx.ResteasyReactiveOutputStream. Because it is not trivial to get right and currently cumbersome to keep in sync, this PR refactors ResteasyReactiveOutputStream, so that it can be re-used by Quarkus CXF and possibly other frameworks needing to pass HTTP responses via a java.io.OutputStream.

In particular, this PR does the following:

Make ResteasyReactiveOutputStream independent of RESTeasy
Move it (along with its ancillary classes) to a separate module in Quarkus that depends only on vertx-web but does not depend on RESTeasy

If the new module will work well, we may consider moving it to Vert.x

Resolves: Refactor ResteasyReactiveOutputStream so that it can be used by Quarkus CXF #40994

ppalaga · 2024-06-05T17:58:22Z

Would you like to review @cescoffier ?

ppalaga · 2024-06-06T15:48:03Z

6f46052: removed the byte-buddy-maven-plugin from the new module

ppalaga · 2024-06-07T08:07:01Z

Not sure about the failure in truststore-maven-plugin on Windows. I see it passing on Linux. Is it perhaps intermittent?

Same with MpRestClientAsyncTest.testIntegrationWithMpRestClient: I cannot reproduce it locally. Is the test perhaps known to be flaky @gsmet @brunobat?

geoand · 2024-06-10T06:24:55Z

Please update the description with information about what this change enables.

Asking because for someone just seeing the PR out of the blue (or looking back at it a few months in the future), there is no context on why this was added.

geoand · 2024-06-10T06:26:45Z

...sy-reactive/server/vertx-java-io/src/main/java/io/quarkus/vertx/java/io/VertxBufferImpl.java

+import io.vertx.core.json.JsonArray;
+import io.vertx.core.json.JsonObject;
+
+public class VertxBufferImpl implements Buffer {


Why is this needed? Don't we already have this class elsewhere?

Thanks for the hint. This one was supposed to be moved from independent-projects/resteasy-reactive/server/vertx/src/main/java/org/jboss/resteasy/reactive/server/vertx/VertxBufferImpl.java, but I forgot to remove the old one. Which I am going to do right now.

There is still one more in extensions/vertx/runtime/src/main/java/io/quarkus/vertx/core/runtime/VertxBufferImpl.java which can be removed in favor of this one. Which I am going to do as well.

I would like to ask you to add a little Javadoc to the classes you are going to use in CXF, as these classes are now no longer going to be internal, so it would be nice for integrators to know a little about what each one does.

Hm... now I see there is also io.vertx.core.buffer.impl.BufferImpl that does some additional bounds checks. I guess the two VertxBufferImpl do not do it intentionally for performance reasons?

I would like to ask you to add a little Javadoc

Haha, I can try

I guess the two VertxBufferImpl do not do it intentionally for performance reasons?

OK, if so, I guess, it won't be so straightforward to move the refactored classes to Vert.x due to our "unsafe" BufferImpl. Therefore, I am going to rename the module to quarkus-vertx-java-io in line with the expectation that it will stay in Quarkus. Also, because the module is now used also in quarkus-vertx and quarkus-websockets-next, I am going to move it to directly under independent-projects.

ppalaga · 2024-06-10T07:08:50Z

Please update the description with information about what this change enables.

Asking because for someone just seeing the PR out of the blue (or looking back at it a few months in the future), there is no context on why this was added.

No problem, done.

ppalaga · 2024-06-10T14:52:41Z

1a6ba5d:

Rebased
The new module quarkus-vertx-java-io is now directly under independent-projects as it is referenced not only from resteasy-reactive but also from quarkus-vertx and quarkus-websockets-next.
Added some JavaDoc - please review

geoand · 2024-06-10T14:56:53Z

cc @mkouba for the websockets-next change

mkouba · 2024-06-10T15:20:54Z

independent-projects/vertx-java-io/pom.xml

+
+    <groupId>io.quarkus.vertx-java-io</groupId>
+    <artifactId>quarkus-vertx-java-io</artifactId>
+    <name>Ancillary classes for using Vert.x with frameworks designed to read/write from/to java.io streams</name>


I'm not so sure we should create an independent project for this kind of stuff. AFAIK there are some drawbacks related to version management, release process, etc.

@gsmet may know more.

Also quarkus-vertx-java-io feels really weird. I'd call it quarkus-vertx-buffer.

I'm not so sure we should create an independent project for this kind of stuff. AFAIK there are some drawbacks related to version management, release process, etc. @gsmet may know more.

+1 about the concern about the version management. I was hoping all was automated given that there are several independent projects already. Anyway, I am open to discuss where the module should be hosted. WDYT @gsmet?

Also quarkus-vertx-java-io feels really weird. I'd call it quarkus-vertx-buffer.

Re-using the VertxBufferImpl was rather a secondary idea. The module was primarily intended to host the VertxOutputStream and perhaps also other java.io stream implementations usable for old frameworks wanting to run on top of Vert.x. I do not care much about the module name. quarkus-vertx-java-io, quarkus-vertx-buffer or even quarkus-vertx-web-utils would work for me.

Re-using the VertxBufferImpl was rather a secondary idea.

In that case something like quarkus-vertx-utils might make more sense...

BTW I also found io.quarkus.resteasy.runtime.standalone.VertxOutputStream. Isn't it a copy of the original VertxOutputStream?

Re-using the VertxBufferImpl was rather a secondary idea.

In that case something like quarkus-vertx-utils might make more sense...

OK, let me rename it to quarkus-vertx-utils unless nobody has a better idea.

BTW I also found io.quarkus.resteasy.runtime.standalone.VertxOutputStream. Isn't it a copy of the original VertxOutputStream?

They are different. The VertxOutputStream from resteasy-reactive-vertx extends java.io.OutputStream rather than org.jboss.resteasy.spi.AsyncOutputStream

I'm not so sure we should create an independent project for this kind of stuff. AFAIK there are some drawbacks related to version management, release process, etc.

I can't remember there being any such drawbacks

I'm not so sure we should create an independent project for this kind of stuff. AFAIK there are some drawbacks related to version management, release process, etc.

I can't remember there being any such drawbacks

There are some version properties in various independent projects, which have to be kept in sync on various places. This PR makes the mess a bit worse by adding two new properties in the new module.
It would be nice to hear what @gsmet thinks.

independent-projects/vertx-java-io/src/main/java/io/quarkus/vertx/java/io/VertxBufferImpl.java

…us CXF fix quarkusio#40994

ppalaga · 2024-06-11T08:51:04Z

64001d4:

Renamed VertxBufferImpl to NoBoundChecksBuffer
Renamed the new module to quarkus-vertx-utils

Remaining open questions:

Is it OK to add a new top level independent project with two new version properties? cc @gsmet

quarkus-bot · 2024-06-11T14:44:10Z

Status for workflow `Quarkus CI`

This is the status report for running Quarkus CI on commit 64001d4.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/smallrye-reactive-messaging-kafka/deployment

✖ io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase.sseStream - History

Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase Expecting size of: [] to be greater than or equal to 2 but was 0 within 10 seconds. - org.awaitility.core.ConditionTimeoutException

org.awaitility.core.ConditionTimeoutException: 
Assertion condition defined as a Lambda expression in io.quarkus.smallrye.reactivemessaging.kafka.deployment.dev.KafkaDevServicesDevModeTestCase 
Expecting size of:
  []
to be greater than or equal to 2 but was 0 within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)

📦 extensions/smallrye-reactive-messaging/deployment

✖ io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector - History

Expecting actual: ["-6","-7","-9","-10","-11","-12","-13","-14"] to start with: ["-6", "-7", "-8", "-9"] - java.lang.AssertionError

java.lang.AssertionError: 

Expecting actual:
  ["-6","-7","-9","-10","-11","-12","-13","-14"]
to start with:
  ["-6", "-7", "-8", "-9"]

	at io.quarkus.smallrye.reactivemessaging.hotreload.ConnectorChangeTest.testUpdatingConnector(ConnectorChangeTest.java:41)

brunobat · 2024-06-11T18:09:58Z

Not sure about the failure in truststore-maven-plugin on Windows. I see it passing on Linux. Is it perhaps intermittent?

Same with MpRestClientAsyncTest.testIntegrationWithMpRestClient: I cannot reproduce it locally. Is the test perhaps known to be flaky @gsmet @brunobat?

That's been failing a bit. Likely related.

gsmet

We can merge this but I would like us to start the discussion to move this to Vert.x as I would like us to get rid of it at some point in the future.

ppalaga · 2024-06-12T08:30:07Z

We can merge this but I would like us to start the discussion to move this to Vert.x as I would like us to get rid of it at some point in the future.

Thanks for merging, @gsmet, I have created a followup #41150

quarkus-bot bot added the area/dependencies Pull requests that update a dependency file label Jun 5, 2024

This comment has been minimized.

Sign in to view

ppalaga force-pushed the i40994 branch from 0fd2d47 to 6f46052 Compare June 6, 2024 15:47

ppalaga mentioned this pull request Jun 6, 2024

Rely on the new vertx-java-io module instead of adapting Quarkus RESTeasy classes on the fly quarkiverse/quarkus-cxf#1407

Closed

This comment has been minimized.

Sign in to view

cescoffier requested a review from geoand June 9, 2024 16:40

geoand reviewed Jun 10, 2024

View reviewed changes

ppalaga force-pushed the i40994 branch from 6f46052 to 23385ef Compare June 10, 2024 07:56

quarkus-bot bot added the area/resteasy-classic label Jun 10, 2024

This comment has been minimized.

Sign in to view

quarkus-bot bot added the triage/flaky-test label Jun 10, 2024

ppalaga force-pushed the i40994 branch from 23385ef to 1a6ba5d Compare June 10, 2024 14:47

mkouba reviewed Jun 10, 2024

View reviewed changes

independent-projects/vertx-java-io/src/main/java/io/quarkus/vertx/java/io/VertxBufferImpl.java Outdated Show resolved Hide resolved

This comment has been minimized.

Sign in to view

Refactor ResteasyReactiveOutputStream so that it can be used by Quark…

64001d4

…us CXF fix quarkusio#40994

ppalaga force-pushed the i40994 branch from 1a6ba5d to 64001d4 Compare June 11, 2024 08:50

geoand approved these changes Jun 11, 2024

View reviewed changes

gsmet approved these changes Jun 11, 2024

View reviewed changes

gsmet merged commit b78bc27 into quarkusio:main Jun 11, 2024
52 checks passed

quarkus-bot bot added this to the 3.12 - main milestone Jun 11, 2024

This was referenced Jun 12, 2024

[quarkus-main] CXF SOAP tests fail with NCDFE for io/quarkus/vertx/core/runtime/VertxBufferImpl apache/camel-quarkus#6179

Closed

Move the classes from quarkus-vertx-utils to Vert.x #41150

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor ResteasyReactiveOutputStream so that it can be used by Quarkus CXF #40995

Refactor ResteasyReactiveOutputStream so that it can be used by Quarkus CXF #40995

ppalaga commented Jun 5, 2024 •

edited

Loading

ppalaga commented Jun 5, 2024

This comment has been minimized.

ppalaga commented Jun 6, 2024

This comment has been minimized.

ppalaga commented Jun 7, 2024

geoand commented Jun 10, 2024

geoand Jun 10, 2024

ppalaga Jun 10, 2024

geoand Jun 10, 2024

ppalaga Jun 10, 2024

ppalaga Jun 10, 2024

geoand Jun 10, 2024

ppalaga Jun 10, 2024

ppalaga commented Jun 10, 2024

This comment has been minimized.

ppalaga commented Jun 10, 2024

geoand commented Jun 10, 2024

mkouba Jun 10, 2024

mkouba Jun 10, 2024

ppalaga Jun 10, 2024

mkouba Jun 10, 2024

ppalaga Jun 10, 2024

geoand Jun 11, 2024

ppalaga Jun 11, 2024

This comment has been minimized.

ppalaga commented Jun 11, 2024

quarkus-bot bot commented Jun 11, 2024 •

edited by github-actions bot

Loading

brunobat commented Jun 11, 2024

gsmet left a comment

ppalaga commented Jun 12, 2024 •

edited

Loading

Refactor ResteasyReactiveOutputStream so that it can be used by Quarkus CXF #40995

Refactor ResteasyReactiveOutputStream so that it can be used by Quarkus CXF #40995

Conversation

ppalaga commented Jun 5, 2024 • edited Loading

ppalaga commented Jun 5, 2024

This comment has been minimized.

ppalaga commented Jun 6, 2024

This comment has been minimized.

ppalaga commented Jun 7, 2024

geoand commented Jun 10, 2024

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

ppalaga commented Jun 10, 2024

This comment has been minimized.

ppalaga commented Jun 10, 2024

geoand commented Jun 10, 2024

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

This comment has been minimized.

ppalaga commented Jun 11, 2024

quarkus-bot bot commented Jun 11, 2024 • edited by github-actions bot Loading

Status for workflow Quarkus CI

Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 extensions/smallrye-reactive-messaging-kafka/deployment

📦 extensions/smallrye-reactive-messaging/deployment

brunobat commented Jun 11, 2024

gsmet left a comment

Choose a reason for hiding this comment

ppalaga commented Jun 12, 2024 • edited Loading

ppalaga commented Jun 5, 2024 •

edited

Loading

quarkus-bot bot commented Jun 11, 2024 •

edited by github-actions bot

Loading

Status for workflow `Quarkus CI`

ppalaga commented Jun 12, 2024 •

edited

Loading