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
[Resteasy-2700] reactor netty server adapter #2693
[Resteasy-2700] reactor netty server adapter #2693
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @crankydillo , this looks very interesting.
In order for merging, we need all contributors to agree with the license agreement; most of you already did that in the past, I believe @abhijith-prabhakar is the only one missing.
@abhijith-prabhakar , can you confirm you agree your contributions are made under the terms of the ASL 2.0 License: http://www.apache.org/licenses/LICENSE-2.0 ?
@crankydillo , would you be interested in adding something meaningful in the documentation (around here https://docs.jboss.org/resteasy/docs/4.6.0.Final/userguide/html/RESTEasy_Embedded_Container.html ) or to write a blog post describing the feature?
Thanks
/** | ||
* Tests SNI capabilities for Netty based JaxRS server. | ||
* | ||
* @author Sebastian Łaskawiec | ||
* @see https://issues.jboss.org/browse/RESTEASY-1431 | ||
* | ||
* TODO what is SNI?? | ||
public class SniTest | ||
{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As for SNI, see the RESTEASY-1431 jira above and https://tools.ietf.org/html/rfc6066#page-6
If this is not implemented, I suggest creating a followup jira after this will be merged. Also better removing this test class completely instead of having it fully commented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR may not suggest it, but our team almost never leaves commented-out code. Apologies for that!
I will see the JIRA and the RFC. If we still feel it should be unimplemented, I will remove this test. Ditto for all other commented out code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asoldano AFAIK, we don't personally need SNI mapping at this time. I removed the test. If you feel this is critical functionality, please let us know.
@crankydillo as for not squash-merging, that's fine. Usually the reason for doing that is making it easy to isolate changes for a given feature/contribution. Would you mind amending the few commits whose commit messages do not start with [RESTEASY-2700] or RESTEASY-2700 already so that they actually start like that? That would help, thanks! |
@crankydillo I've re-started the CI tests. The failures on Windows are frequently environmental (we're investigating them), though the org.jboss.resteasy.test.HeaderTooLongTest failure in one of the linux run is possibly relevant. Should it fail again, it will have to be addressed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your contribution! Overall I think this looks fine. It looks very similar to the Netty adaptor which is nice.
The biggest change we need to make is not using slf4j and using jboss-logging instead.
resteasy-dependencies-bom/pom.xml
Outdated
@@ -106,7 +106,7 @@ | |||
<version.org.jboss.marshalling.jboss-marshalling>2.0.6.Final</version.org.jboss.marshalling.jboss-marshalling> | |||
<version.rest-assured>3.3.0</version.rest-assured> | |||
<version.org.springframework>5.2.0.RELEASE</version.org.springframework> | |||
<version.io.projectreactor>Dysprosium-SR12</version.io.projectreactor> | |||
<version.io.projectreactor>2020.0.4</version.io.projectreactor> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not critical, but 2020.0.5 was release a few days ago. We may as well upgrade it.
<dependency> | ||
<groupId>org.jboss.logging</groupId> | ||
<artifactId>jboss-logging-processor</artifactId> | ||
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be marked as <scope>provided</scope>
. Not required, but <optional>true</optional>
could also be added. This is a compile time only requirement.
<dependency> | ||
<groupId>ch.qos.logback</groupId> | ||
<artifactId>logback-classic</artifactId> | ||
<version>1.2.3</version> | ||
</dependency> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this a required dependency? Ideally we should not be requiring a log manager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming this will be rectified when I change to jboss-logging
..
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay excellent thanks!
<dependency> | ||
<groupId>io.projectreactor.tools</groupId> | ||
<artifactId>blockhound</artifactId> | ||
<version>1.0.3.RELEASE</version> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this dependency not in the BOM? If it is we should likely remove the version here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will check this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will just remove it since it doesn't look like we used it.
...y/src/main/java/org/jboss/resteasy/plugins/server/reactor/netty/ReactorNettyJaxrsServer.java
Show resolved
Hide resolved
@Message(id = BASE + 10, value = "Chunk size must be at least 1") | ||
String chunkSizeMustBeAtLeastOne(); | ||
|
||
@Message(id = BASE + 12, value = "Exception caught by handler") | ||
String exceptionCaught(); | ||
|
||
@Message(id = BASE + 15, value = "Failed to parse request.") | ||
String failedToParseRequest(); | ||
|
||
@Message(id = BASE + 20, value = "response is committed") | ||
String responseIsCommitted(); | ||
|
||
@Message(id = BASE + 25, value = "Unexpected") | ||
String unexpected(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These do not appear to be used and should be deleted.
...actor-netty/src/main/java/org/jboss/resteasy/plugins/server/reactor/netty/i18n/Messages.java
Show resolved
Hide resolved
...actor-netty/src/main/java/org/jboss/resteasy/plugins/server/reactor/netty/i18n/Messages.java
Show resolved
Hide resolved
// we can consider adding the feature. | ||
// TODO we do not set 'ChannelHandlerContext' (see resource method) | ||
// Are we supposed to be doing something there? | ||
//@Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor nit, could we keep the @Test
here and add @Ignore
please? Or we could just delete it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not minor to us! I almost certainly will remove this.
Thanks!
I will add to this PR a commit that targets that HTML page. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the quick feedback! I should be able to incorporate the requested changes within a few days.
|
||
@Override | ||
public void write(int b) { | ||
byteSink.emitNext(Tuples.of(new byte[] {(byte)b}, new CompletableFuture<>()), EMIT_FAILURE_HANDLER); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good eye. I will look into this. It's definitely fair to say we were focused on asyncWrite
...
if (!started) { | ||
byteSink = byteSinkSupplier.get(); | ||
parentResponse.committed(); | ||
started = true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We only needed double-checking if it can be called from different threads 'simultaneously', right? Is that a possibility?
package org.jboss.resteasy.plugins.server.reactor.netty; | ||
|
||
public interface Logging { | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this was not supposed to be committed.
import java.util.List; | ||
import java.util.Map; | ||
|
||
// TODO copied from netty4. Do we make common? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will clean this up. However, if I do end up with anything beyond the most trivial copying, should I just mention them by name (@author
)?
import org.slf4j.Logger; | ||
import org.slf4j.LoggerFactory; | ||
|
||
public class ReactorNettyContainer { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Honestly, it was 6 months ago when I wrote this. I believe it is only used from test. I did get the pattern from other adapters. I will need to see if our spring-boot starter work leverages it.
root = rootResourcePath; | ||
if (root != null && root.equals("/")) { | ||
root = ""; | ||
} else if (!root.startsWith("/")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What would you like it to throw? I generally prefer to bubble NPE. However, I would likely have used Objects#requireNonNull
on line 87. Should it set it to null
or the empty string when the supplied argument is null? I will check the javadoc to see if it gives guidance.
...y/src/main/java/org/jboss/resteasy/plugins/server/reactor/netty/ReactorNettyJaxrsServer.java
Show resolved
Hide resolved
...actor-netty/src/main/java/org/jboss/resteasy/plugins/server/reactor/netty/i18n/Messages.java
Show resolved
Hide resolved
...actor-netty/src/main/java/org/jboss/resteasy/plugins/server/reactor/netty/i18n/Messages.java
Show resolved
Hide resolved
// we can consider adding the feature. | ||
// TODO we do not set 'ChannelHandlerContext' (see resource method) | ||
// Are we supposed to be doing something there? | ||
//@Test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not minor to us! I almost certainly will remove this.
Hi @asoldano . Did you re-run it? I will see if I can replicate on my home box, which is Linux Mint-based. |
…andling improvements (#3) * Build UriInfo from full URI * Tests to verify UriInfo * Server error handling update * Update server-adapters/resteasy-reactor-netty/src/test/java/org/jboss/resteasy/plugins/server/reactor/netty/ClientUncheckedErrorTest.java Co-authored-by: Sharath Srinivasa <sharathsrinivasa@users.noreply.github.com> Co-authored-by: Samuel Cox <crankydillo@gmail.com> Co-authored-by: Sharath Srinivasa <sharathsrinivasa@users.noreply.github.com>
Co-authored-by: Abhijith Prabhakar <abprabhakar@paypal.com>
2efcec7
to
6f0855a
Compare
Yes, I agree |
Some notables: use jboss-logging, add HEAD test, deal with several potential NPEs, use JDK `Collections`, and remove SniTest. SNI support can be added later (when someone needs it:)
6f0855a
to
f14a805
Compare
The build complained about a checkstyle error. I force-pushed a fix for that. I'll push the doc changes to the embedded-server HTML page in just a bit. |
@asoldano I pushed the doc addition in this commit. |
I see the Error:
|
@crankydillo , I'm not sure how much this will help, but here area few thoughts I have on the topic:
|
@asoldano Thanks for the tips. I can reproduce the failure pretty easily just by increasing the size of the header value. I get the same error, which I hope means it's the same problem:) I'm thinking the issue may be with reactor-netty. I'm working on a failing test (and hopefully fix) for their codebase. We'll see. I must say though that this 413 behavior isn't 'super critical' for us. From the customer perspective, it ultimately is a request failure. However, it would be nice to always get a nice response. As @anilgursel points out in the test comment, one thing we probably want to do here is make the max header size configurable. That may prove more important than a nice message for super large header values.. |
[reactor-netty issue](reactor/reactor-netty#1584) which will soon be documented on their codebase. Basically, they fail to read the entire request before closing the connection. This is something we are willing to live with for now.. The test was adjust to show that if the header value is too long (but not too too long:), the server will respond with a 413. While doing this, I noticed that reactor-netty has added its own 'idle timeout' functionality so we'll leverage that instead of registering custom netty handlers. [RESTEASY-2700](https://engineering.paypalcorp.com/jira/browse/RESTEASY-2700)
c5ce799
to
6b3e0a9
Compare
@asoldano Looks like all are passing except for windows. You mentioned you often have issues there. I just glanced, but it looks the errors are related to vertx. Hopefully, there is nothing in this PR affecting that. |
@crankydillo thanks a lot! Yep, the resteasy-vertx failures in windows are unrelated, actually those are being addressed elsewhere soon. |
I thought I covered all of @jamezp comments, but if I missed one, I trust he'll let me know:) |
@asoldano Apologies for pestering. Any ideas on a time-frame for this? :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've only really reviewed my previous comments which I still have a couple questions on. I can give it a full review again though as well if needed.
if (!started) { | ||
byteSink = byteSinkSupplier.get(); | ||
parentResponse.committed(); | ||
started = true; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where have we ended up on this? Do we know if the output stream is not thread safe?
root = rootResourcePath; | ||
if (root != null && root.equals("/")) { | ||
root = ""; | ||
} else if (!root.startsWith("/")) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ronsigal Do you know if a null
root resource is valid?
package-private. [RESTEASY-2700](https://engineering.paypalcorp.com/jira/browse/RESTEASY-2700)
I think I addressed everything. If I missed something, please let me know. |
@crankydillo thanks for the update. I think we are fine for merging this now. As the release might not come very soon, we might still have a bit more of time to fix problems if there's need to. Thanks! |
We have an investment in RestEasy and Reactor-Netty. As such, we'd like to leverage reactor-netty as the server behind RestEasy's JAX-RS-server implementation. Once this is merged, we will create a resteasy-springboot contribution that leverages this work.
Our hope is that you will not squash-merge if that's possible as this work was done by all the people you see in the commit history.