servlet: Implement gRPC server as a Servlet #8596
Conversation
|
@niloc132 thanks a lot for your link. We'll definitively look at them. Unfortunately we use javax.* in our code and changing this will be difficult. @dapengzhang0 anything missing in this PR ? |
|
@hypnoce given that we're coming up on a year of this PR having been updated, I'm going to be moving forward with publishing a complete version (i.e. |
@niloc132 |
Yes, when I get the chance to complete this work, I'll update the ticket and this PR. I'm sorry that I havent had time to do so, but as mentioned above, we've shipped the |
servlet/src/jettyTest/java/io/grpc/servlet/JettyTransportTest.java
Outdated
Show resolved
Hide resolved
servlet/src/jettyTest/java/io/grpc/servlet/JettyTransportTest.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| @Override | ||
| @Ignore("regression since bumping grpc v1.46 to v1.47") |
There was a problem hiding this comment.
Is this still an issue in v1.51? If you filed an issue for this regression, it would be nice to reference that number.
There was a problem hiding this comment.
I filed #9308 to track the issue (which can be referenced here in the annotation). Seems there's no update to the issue. One can bump to the latest grpc version to see if the regression is still there. Otherwise need to do a bisect from v1.45 (the annotation mentioned v1.46 but actually it was bumping from v1.45) to v1.47 to find which change has caused the issue.
There was a problem hiding this comment.
I've merged up to latest and the test still fails, incremented the version.
servlet/src/main/java/io/grpc/servlet/AsyncServletOutputStreamWriter.java
Outdated
Show resolved
Hide resolved
servlet/src/test/java/io/grpc/servlet/AsyncServletOutputStreamWriterConcurrencyTest.java
Outdated
Show resolved
Hide resolved
|
@larry-safran Thank you for the review, I'll try to follow up shortly to make those changes. Can you confirm that make those will lead us down the path of getting this merged? I'm a little hesitant to invest further at this point. |
larry-safran
added
the
kokoro:force-run
grpc-kokoro
removed
the
kokoro:force-run
|
Yes. We can merge if none of the tests are unexpectedly failing and will fix them if they are. |
|
FYI, larry-safran was taking a look because I was saying I wanted to get this over the finish line. This has not been merged for far too long; it'd serve the code and everyone better to get it merged. For the small nits and fixes, I am completely fine with doing them in follow-ups or us maintainers pushing the fixes directly to this PR. I'm also fine with disabling a servlet-specific test or three (like #9308) in order to get it in. (:-/, #9790 looks to have noticed new style failures which are failing the CI.) |
|
Thank you - I don't mean to sound pessimistic, and will get these done ASAP. I'll have a few followups to propose that we've already made too, but didn't want to further burden this PR with. |
ejona86
added
the
kokoro:run
grpc-kokoro
removed
the
kokoro:run
|
@niloc132 Please go ahead and merge this. |
|
@larry-safran, they don't have permission to merge this. For non-maintainer contributions, once we get two approvals we do the merge (typically the one who gives the second approval). (Make sure to clean up the commit message when squashing.) |
This is an updated version of the code found at #4738, merged up to latest, tests restored from a different branch, and with a few of the remaining review comments addressed as well.
The tests run during the build against Jetty 9, 10, 11, Tomcat 9, 10, and Undertow 9. Note that Jetty 10 and 11 are largely the same except for the javax/jakarta package changes). There are still several tests marked as FIXME, and several more ignored due to how servlets function differently than something entirely self-contained like grpc-netty. Note also that while Jetty 9 works, it requires a workaround for missing support for writing trailers - I am amenable to dropping this workaround or documenting it in some way for downstream users, but without this Jetty 9 can't be tested in Java 8. Note also that a standard Jetty 9 installation will not support this use case, but it either needs to be lightly customized, or used as an embedded web server.
A second project is also added, grpc-servlet-jakarta, which consists of build logic to rewrite package names for
javax.servletimports and replace them with their correspondingjakarta.servletimports. This also removes the Jetty workaround, as it is unnecessary (Jetty 11+ supports thejakarta.servletAPIs, which necessarily means that trailers are supported using the standard API calls).--
As a new contributor to this project, I anticipate that I have made some mistakes in trying to bring my parts of this patch up to the standards expected for contribution, and it is quite possible that I missed some conversation where it was decided that this wouldn't make sense to merge to grpc-java itself. If that is the case, I will be releasing this separately - outside of tests, there should be no other changes to the modules in grpc-java itself. With this released one way or the other, my intention is to add grpc-web support here as well, to allow a standalone (i.e. without Envoy) Java web server to provide static html/js content and grpc services, which otherwise requires ~3 processes (and potentially one more to support non-SSL streaming to the browser).
Inspecting my own code, I think it makes sense to improve usability of the tests in IntelliJ, and add README.md files to each of grpc-servlet and grpc-servlet-jakarta.