servlet: Implement gRPC server as a Servlet

[niloc132]

This is an updated version of the code found at https://github.com/grpc/grpc-java/pull/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.servlet imports and replace them with their corresponding jakarta.servlet imports. This also removes the Jetty workaround, as it is unnecessary (Jetty 11+ supports the jakarta.servlet APIs, 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.

[linux-foundation-easycla[bot]]

The committers are authorized under a signed CLA.

• :white_check_mark: ZHANG Dapeng (a0c92df6a6f29c0617f9c21c96651f2641423534, 87193772ece028a9e971095d2b4c698905c2575d, 46d00ec92fb34dd088c243f80ee97029d606fbe7, 4fa6271d3abe53de57e091d1b9527781c93318ac, e793c7cce4b870428bfd92db8ad7052c0bac3dfe, 393fcf63ed97ce7efa2a2fbe2e94b3d220586d12, 706a3c9a65848bcaf03de0e27aada31029dc71a0, 5e11674a92d712a7bad01930bd3432349ddc4fe2, 17769174d176db77874a0f0d0dbc72764fe76357, bb3a720b9154efebd09346852a932378cf798dfc, 4c7cd4febc281aac0872ce9c25c55e8c7af73858, af7f6a3a7fcc626281a557e7348b4fc98c68cc47, 2fc04fc11be5e04649c847afadd88d7cd68beb17, 6fb4fc2025b32f18e5b55022d7cfe9dd4126800e, 8b78b6d42d5edca1609c9d510c57769836fa97b4, 680872bb5a2ea4360d365879bbe93a82d1ccb650, b3473df13de7b7255034a2b9b9dfd615418b6d45, 372886608b06198122bfa2ed84342ef927213226, 58fce6320716b7fb6c091a64762fe22504a05ae0, 5b02064c11c8c3bf1e4de19696a3fa77064bff75, d59f81e7a7f21ccae75376e2d26c42701ba532c2, 5b7496e993e0af5b47dfef7ba1ab1563510b6492, e98e4f84baf380c076cdeea169d2f16504ce53d2, 6aa26e42427bd028cf92f7dd8b2963e880bb34a9, c3dfaba2bb1cde4bb64d6cb91ff7fa7ea923f4e4, 44ea5f3b3f2b976adadcc4ec441659b33f0a0614, ef5f87916b3a2b766613ce734a5b471ac2a379fe, 47e3e955aa2c637c4a7b782f5d64b48a4282f081, c5fc2bd6d971c80c37b3912381f61058253d5e32, 63e8cb2ff9987ead928f7d5551b63543c3cda9ba, 986a885dd775c94f4797b669b14f670646c23e50, 3fd4ca85e9c61c98230031b05022483d5a39b764, 7d8410f9e16d32b6ac5e0a923ba80a5858ae9247, a39be4c0832cb364337b0b25da9761fcb8b85955)
• :white_check_mark: Chengyuan Zhang (09967c106067a6b3b883cebfbc722f2d078dbb6d)
• :white_check_mark: Colin Alworth (a53114bbe14000ac02ca58785cf181fdc75e8284, ae0085f082e8f8069b5f6c61ad6148b781decba2, 9a02c822399034426cd20e6f3f1e63c225f32ba0, 7977de6a239cc59a6fe069719698fa32ec84b02d, 811fcbed0983617d27cfb2fa9200e3afcb627ffa, ccbfd8d17d7adfc229e550d8a4c06cad765e3de2, 5d017a3992bef357206307317498d4b96fbde026, 0180d293bc74c8d3799a9f7d30ae22bb8e96460e, 7bb191c2dcfe130142f6f0de02725b71929632e5, 59d97548b36c73169677e871205c6e980a2a5717, 1f4b0cae255687de5293af69fecaeb81c84fbb6c, ccc5cd6aaeec970a5e1b4bd91e5ed1a9d909b92b, 7d90af877d930285e7f5cb304682345a0f9e79c8, aea3351dcbfe4e9f46edd480c6c9eb551fff9c85, 70ca9200c698c38a6584a1c8c3b684ebae6db177, 83d405e910da8959330f7cc7fc88fd5829ef6735, 18d20d3f65b96902cc71a24d624ab60bc031f01d, f4c0c8739a0743b36f81ec1043c8aecba2c15c64, b12370eb0d5141e797e6dac48240ed447ea58936, dea10c6e4fbd36550d6a1602ebb2a010df81064e, 2154764805412733c6552a7b677615cd48dc2610, bf5c225242eb89b53d288feffa168628ae9f716c, a0b5852d14a63152a4926486b8de9386852671ed, 512a2d32547bf612248dd909fb6764b16c2e9af1, 4bed353255da8084d8eb8058ccc6ae73e78b6ab5, 907f432c74777392a192a5617dd9b5b4a7f2ecde, 75f9b82578afbfda2bf9b082087352ff88492a3b, cf3decb481f8d5b8e5670741b11df06b9329ce8a, 00b8f762b46021517c05b0bd9f71513bdf1cd9e2, 35e0af2ef638864e4345c53c1440706029e8bfd0, 0c5664cd193857be7188eae4ffe646429225294c, 8162d6efd2a0b5222e485619e87bb79790bfe774, 8e25c4c6dffc4fae11029caabc61e13044178849, 3bcd0af46ed6b09314c21fdef330afc4feec8550, 034771f7f05b00505841cb3737941481aa35cb54, da85cf8661b87d1c5a34a5a0cda47460a66f23ad, f84a156ba1b196680eb5ad6e5d9ecca8633f2ea1, f7677f6be44b73f6e313d5755dbe44355e716b73, dc2ddcf7d16e00b83b806b458df3259eb8fae1e6, cc9eb3fb16f17de8829ede42d103e7ef66883ba8)

[dapengzhang0]

@niloc132 Really appreciate it for pushing forward my almost languishing PR due to limited bandwidth. Can you please sign the CLA so we can start to review? Let me know if you have issues signing.

[niloc132]

Sorry for the delay, the person responsible for signing on my behalf is out of the office today, we should have it back soon.

Thanks for your excellent work in making this possible - I started using your branch about a year ago and had good success with it, but have only intermittently had the chance to merge it up to date further than what I needed it to be at the time.

[niloc132]

Could it be done completely reflectively? I really don't want to have strong dependency on container-specific classes (e.g. org.eclipse.jetty.server.*) here.

(I can't seem to reply to your message here, so making a general reply instead.)

Perhaps, though you'll notice that this class only has a compile-time dependency on Jetty, and won't require a runtime dependency unless Jetty is already in use. This was written this way to avoid the need for explicit reflection, so as to make it easier to understand, but I'm of the opinion at this point that it isn't worth the trouble. My self-review pointed this out too, jetty 9 might just be more trouble than it is worth.

I had hoped to keep Jetty 9.4 support, as it seems to have better http2 support than the other two servlet containers you had written tests for, and it still supports Java 8, but the jetty project isn't interested in patching this bug as part of their release, and there are a few ways that this workaround can still go wrong, so I'm inclined to remove it entirely, except perhaps as a footnote to enable embedded jetty 9.4 support.

[dapengzhang0]

Sorry didn't get a chance to focus on this in the past weeks, will find time in December.

[dapengzhang0]

@niloc132, I pushed some small fixes to your branch.

[niloc132]

We've copied these changes directly into the repo I'm working on, with one minor alteration so that a servlet-based grpc-web adapter can also use the same ServerTransportListener etc. This is certainly the wrong way to approach this problem, but I don't yet have the right one - I'll come back to the grpc-servlet with a followup as we finish implementing the other pieces we need on top, but my hunch is that the GrpcServlet will mostly be replaced with a GrpcFilter that handles application/grpc calls, and lets the others pass through to other appropriate servlets/filters.

[niloc132]

Jetty9 workaround and tests have been removed, and the jakarta build correctly publishes an artifact now as well.

I'm still struggling with several tomcat tests that intermittently fail, I'll spend a little time on them over the next few days, but will probably err on the side of ignoring them (to be revisited as the tested tomcat version is updated) - does that seem reasonable?

Thanks for taking the time to review this, I appreciate it.

[dapengzhang0]

Jetty9 workaround and tests have been removed, and the jakarta build correctly publishes an artifact now as well.

Thank you so much!

I'm still struggling with several tomcat tests that intermittently fail, I'll spend a little time on them over the next few days, but will probably err on the side of ignoring them (to be revisited as the tested tomcat version is updated) - does that seem reasonable?

Yes, if it's not easy to fix/triage, for the moment we will ignore them and I'll file a tracking issue later, and investigate in the future. In the past, I filed a number of tomcat bugs/enhancement related to async servlet over HTTP/2, some of the intermittent failures took me a lot of time to find a minimal reproducible verifiable example, it was quite painful.

[niloc132]

I've pushed another commit where i set another three flags to instruct tomcat to disregard other cases where the client is too chatty for its likes. Several dozen test iterations later, I feel confident in reporting that none of the three intermittent tests are failing any longer.

[niloc132]

There were warnings from java 9-16 on tomcat's illegal reflective access on startup in the tests, I added the appropriate --add-opens and validated that tests pass cleanly on Java 11 and 17.

[niloc132]

Is there anything I can do to help get this into the right shape to merge and release it?

[dapengzhang0]

Is there anything I can do to help get this into the right shape to merge and release it?

@niloc132 It's almost in the right shape now. I'm trying to add some unit tests. I'm actively on it.

[niloc132]

Thanks - if there is something you'd like me to help out on, leave a note and I'll get to it as soon as possible.

We're doing some experimenting with grpc-web in another ServletAdapter-like class - this makes more sense if you can share the transportlistener, streamtracerfactories etc between them, but I don't have a pattern I like well enough yet to propose here. For the moment at least, we'll maintain our own fork as we iterate this, and propose something when we have some options to suggest, see what fits best into the rest of the project here.

We're also trying using a single Filter to handle all mapped services rather than bind a servlet to each path or bind a specific servlet for each service, so that unhandled paths (or non-grpc calls) can just fall through to other filters or servlets. This also makes it straightforward to create separate handlers for gRPC calls based on the same transportlisterner/streamtracerfactory/etc, as for example websockets* can't be configured consistently across servlet containers to start from a filter/servlet.

* Websockets of course are not a standard grpc/grpc-web transport mechanism, but https://github.com/improbable-eng/grpc-web/ offers a handy client transport that allows a browser to connect to streams with binary data through a websocket proxy. This has also made localhost web development easier, as http2 doesn't work in a browser without tls. I don't imagine that the grpc-java project will be interested in the in-process Java implementation of that transport that we've developed, but we are going to make it generally available for use cases such as ours.

[niloc132]

Checking in on this - are there other changes I can make to help get this merged, or is there a reason we think it might not belong?

I've tried to update it locally to v1.46 or so, but despite cleaning I am consistently getting an error locally from the netty-shaded project, with both java 8 and 11.

io.grpc.netty.shaded.ShadingTest > tcnative FAILED
    java.lang.UnsatisfiedLinkError at ShadingTest.java:121
        Caused by: java.lang.IllegalArgumentException at ShadingTest.java:116

java.lang.UnsatisfiedLinkError: failed to load the required native library
	at io.grpc.netty.shaded.io.netty.handler.ssl.OpenSsl.ensureAvailability(OpenSsl.java:596)
	at io.grpc.netty.shaded.io.netty.handler.ssl.SslContext.newClientContextInternal(SslContext.java:830)
	at io.grpc.netty.shaded.io.netty.handler.ssl.SslContextBuilder.build(SslContextBuilder.java:611)
	at io.grpc.netty.shaded.ShadingTest.tcnative(ShadingTest.java:121)

I'm not sure if there is a specific environmental issue here, since it appears that CI builds aren't affected, and I also get this failure from building at current master.

In the latest CI run for this branch however, I see a failure I wasn't expecting for Java 11:

io.grpc.testing.integration.NettyFlowControlTest > largeBdp FAILED
    java.lang.AssertionError: Window was 688124 expecting 314570
        at org.junit.Assert.fail(Assert.java:88)
        at org.junit.Assert.assertTrue(Assert.java:41)
        at io.grpc.testing.integration.NettyFlowControlTest.doTest(NettyFlowControlTest.java:152)
        at io.grpc.testing.integration.NettyFlowControlTest.largeBdp(NettyFlowControlTest.java:102)

However, this is netty specific (and this code shouldn't be affected by the PR), so I'm not clear why this would start failing now.

---

I have a working prototype to propose as a follow-up for this, which provides a grpc-web in-process proxy, handling only translating the relevant differences between grpc and grpc-web, which mostly so far has boiled down to rewriting the content-type header and correctly serializing trailers as a final http payload. This seems to be working for unary and server-streaming (as browsers can't yet support any client-streaming over either xhr or fetch) with http/1.1, I'll be testing http2 soon (with https, etc). Additionally, only the application/grpc-web content type is supported so far, though adding support for grpc-web-text shouldn't be too difficult.

I know there is an existing (though recently marked "on hold") java servlet implementation of grpc-web at https://github.com/grpc/grpc-web/blob/master/src/connector/, my prototype contrasts with that by using a servlet Filter to hand off the processing to the existing grpc servlet. Among other things, this has the advantage of not needing to re-implement message handling, and allows the existing grpc servlet to call into the normal grpc-core wiring to actually invoke the grpc service method (the linked connector creates service stub instances by reflection, which prevents ServiceDefinitions having the expected control over how things are actually wired up, which in turn means that server interceptors don't actually run, etc). This new filter, in conjunction with the current pull request, should be able to replace the existing connector project.

On the other hand, the filter in its current form is rather specific to the current GrpcServlet implementation, taking care to manage only headers that the expected implementation will expect, produce, so it might make sense to keep them together.

Testing is also a question, I'm not at this time familiar with any complete grpc-web client written in Java. I don't presently have plans to need this outside of testing this very filter, so my own work might remain limited to automated integration tests of a browser client connecting to the grpc-web filter performing an in-process proxy to the actual servlet.

...All of this being a long-winded way of saying that until this PR merges, it makes no sense to land this code to grpc/grpc-java or grpc/grpc-web directly, but that I should plan on expanding our grpc-java fork to include this new aspect.

[dapengzhang0]

@ejona86 Any bandwidth for the final review? It's becoming more and more difficult to make the PR up-to-date as there could be regressions in the main branch that's only impacting grpc-servlet such as #9308 .

@niloc132, I could not push new commits to this PR anymore. Did you disallow edit by the maintainers?

[niloc132]

The checkbox for edits by maintainers is still checked, and I made no deliberate change on my end. Push to a branch on your repo and I'll pull the commits over directly?

[dapengzhang0]

Push to a branch on your repo and I'll pull the commits over directly?

Thanks. @niloc132 Please pull/redo the 4 commits from https://github.com/dapengzhang0/grpc-java/commits/servlet-niloc132 b13d505d06445bcdcfd30e398be9f57871a69ea3 639084a17768d24c31fa7305f74d8ff9b6c24538 7ae71dc8cb536fdfe60d3e57f6f91c51ceaad0ba 5829381162fd38c2f6fd68c5b3c4472f0e707e47

[niloc132]

Done - hopefully updating this will let this work again for you to push, otherwise just ping me and I'll update it again.

[hypnoce]

Thanks for all this effort. Is there anything blocking the merge (besides the failed kokro jobs) ? I'm really interested in this development as it can really simplify operations for us by sharing the same servlet container, same port, for both standard HTTP/REST calls and GRPC calls.

[niloc132]

If you're interested, we have published the jakarta variation of this repo in our own groupid: https://search.maven.org/artifact/io.deephaven/deephaven-grpc-servlet-jakarta/0.14.0/jar. We also have another jar we publish, also licensed apache2, which re-implements the https://github.com/improbable-eng/grpc-web/ websocket proxy in Java, running in-process.

In the next month or so we will be shipping an in-process grpc-web proxy as well, so that http2 browser clients can connect directly to grpc services. We're also working on a variant of the above websocket proxy that treats the websocket more like a "channel" and less like a "stream", to allow many concurrent bidirectional binary streams (in contrast with the https://github.com/grpc/grpc-web project, which can only stream text). Feel free to contact me off-list for more discussion.

[hypnoce]

@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 ?

[niloc132]

@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. javax.servlet as well as jakarta.servlet) in the next month or two, plus the additional enhancements (grpc-web support, websocket transports, etc) that we've developed. I'll update the linked ticket and this pull request with that information once I've finished this process.

[rufreakde]

@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. javax.servlet as well as jakarta.servlet) in the next month or two, plus the additional enhancements (grpc-web support, websocket transports, etc) that we've developed. I'll update the linked ticket and this pull request with that information once I've finished this process.

@niloc132 Will you update this ticket here then as well? Any news?

[niloc132]

@rufreakde

Will you update this ticket here then as well? Any news?

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 jakarta.servlet jars to maven central already.

[niloc132]

@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]

Yes. We can merge if none of the tests are unexpectedly failing and will fix them if they are.

[ejona86]

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.)

[niloc132]

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.

[larry-safran]

@niloc132 Please go ahead and merge this.