Grpc-java: Add an ability to run protobuf server as a Servlet 4

[vlsinitsyn]

Pls add an ability to run a protobuf v3 server as a Servlet 4 on any server with the Servlet 4 support.

[ejona86]

Dump from what I wrote on SO to have the information in one place:

It does seem feasible to use the async servlet APIs to implement a gRPC server (as an alternative to the Netty server), except possibly for trailers. It's not 100% clear how to send trailers when the server is HTTP/2, since a common technique with HTTP/1 was for the servlet to manually perform chunked encoding which does not exist in HTTP/2.

[cs224]

Could someone please give a status update on this topic? Thanks!

[ejona86]

No progress. I still don't know how to send trailers in this environment.

[jnehlmeier]

JavaDoc for Java EE 8 / Servlet 4.0 describes an API to set trailers on a response.

https://javaee.github.io/javaee-spec/javadocs/javax/servlet/http/HttpServletResponse.html#setTrailerFields-java.util.function.Supplier-

[ejona86]

@jnehlmeier, thank you!

I'll note one part of the docs that could be an annoyance, but I think they are talking about the remote, not the servlet (in which case we're fine to ignore it):

The RFC requires the name of every key that is to be in the supplied Map is included in the comma separated list that is the value of the "Trailer" response header. The application is responsible for ensuring this requirement is met. Failure to do so may lead to interoperability failures.

If it is for servlet interoperability we can still live with it, but it requires the application to help us out. Go already has to deal with something similar when using the Go-provided http/2 server.

[hofmanndavid]

@ejona86 can you elaborate what exactly you don't know how to do using servlets? I want to help implement this feature.

Is not this what you are referring to?

@WebServlet(asyncSupported = true)
public class MyServlet extends HttpServlet {

    @Override
    protected void doPost(HttpServletRequest req, HttpServletResponse resp) throws ServletException, IOException {
        AsyncContext asyncContext = req.startAsync(req, resp);

        // here you pass this asyncContext to any thread worker who
        // will be querying the REQ data
        // then you can send a response like

        ((HttpServletResponse)asyncContext.getResponse()).setHeader("Transfer-Encoding","chunked");
        asyncContext.getResponse().getOutputStream().write(new byte[]{});// write your bytes here
        asyncContext.getResponse().getOutputStream().flush();
        ((HttpServletResponse)asyncContext.getResponse()).setTrailerFields(() ->{
            return new HashMap<>(); // add any trailing headers
        });

    }
}

If there is something specific that servlet is not doing for you lets discus explicitly and I'll help.

Even if servlet spec does not allow to run gRPC server, we can still provide enough adapters so that it can run on top of jetty, undertow and tomcat. In that way at least servlets and gRPC can share the same port

An example of what I mentioned was already in the works, see wildfly mailing list

[ejona86]

@hofmanndavid, I think it would be (relatively) easy to make a servlet that would handle the I/O for gRPC. Any questions remaining in my mind are mainly on the nicest way of configuring the thing.

It would implement InternalServer and call ServerListener each inbound RPC. There's a handful of HTTP-isms that we've not split out of NettyServerHandler.onHeadersRead like we have with Http2ClientStreamTransportState, but that's not too big of a deal. For the while it could just be duplicated code.

[hofmanndavid]

Okay, Will try to get back when I have something. give me 2 weeks.

[ejona86]

CC @dapengzhang0

[dapengzhang0]

The "Trailer" in the headers is required by RFC7230 (HTTP/1.1) . So hopefully the "interoperability failure" will only occur for HTTP/1.1 clients, and HTTP/2 clients can still interoperate.

The RFC requires the name of every key that is to be in the supplied Map is included in the comma separated list that is the value of the "Trailer" response header. The application is responsible for ensuring this requirement is met. Failure to do so may lead to interoperability failures.

If it is for servlet interoperability we can still live with it, but it requires the application to help us out. Go already has to deal with something similar when using the Go-provided http/2 server.

[ejona86]

The "Trailer" header applies equally in HTTP/1.1 and HTTP/2. HTTP/2 didn't change those semantics.

[hofmanndavid]

Quoting from the protocol doc

Some gRPC implementations may allow the Path format shown above to be overridden, but this functionality is strongly discouraged. gRPC does not go out of its way to break users that are using this kind of override, but we do not actively support it, and some functionality (e.g., service config support) will not work when the path is not of the form shown above.

The initial problem I am seeing when trying to use a servlet container instead of netty is that netty owns the root path /*
In a servlet container only one deployed artifact will be able to access the root path /*, the others will have a 'contextPath' of their own.

So. We are dealing with multiple applications running in a servlet container. eg.
/ --> one-roor-app.war (app1)
/InternalService/ --> internal-app.war (app2)

There is a rule for request mapping in servlets and is that the longest path that matches is the one used to route a request to the proper app.

In case both apps will use grpc, and is what should be supported. This will happen:

1. app1 exposes a grpc called GreetService and we will be able to route it properly as it owns the root path. OK.
2. app1 exposes a grpc called InternalService and the servlet will route the request to app2. NOT_OK.
3. app2 will want to expose GreetService too!, but it will always be routed to app1. NOT_OK.

For grpc to work in a servlet container the proper way to support it IMHO is to:

1. Allow the client to add a prefix to all service names. Preferably as an optional value set when creating the channel.
2. Allow the server to be aware of that prefix.

It would be desirable to have an optional parameter in the proto file that will trigger small client code generation for adding the prefix (maybe not optional if defined in the proto file)

This will lower the entry barrier for grpc usage inside enterprises that rely a lot in application servers.

Is the reasoning correct or is there a fundamental problem?

[dapengzhang0]

@ejona86 you're right.

The "Trailer" header applies equally in HTTP/1.1 and HTTP/2. HTTP/2 didn't change those semantics.

I read https://tools.ietf.org/html/rfc7230#section-4.4 again, it says "Trailer" header is required
"When a message includes a message body encoded with the chunked transfer coding", which means
Transfer-Encoding:chunked is in the headers, is that grpc not the case?

[ejona86]

In a servlet container only one deployed artifact will be able to access the root path /*, the others will have a 'contextPath' of their own.

Yep. I was aware this was going to be a problem. I think it will remain a problem for the while. I'd like to focus on getting '/' webapps working first.

In case both apps will use grpc, and is what should be supported

I don't think we'll have much of a problem here, since it's likely only the '/' app can serve gRPC. The paths involved almost always end in /, so the only case there could be a collision is if a web app wanted to be deployed at '/myproto.package.MyService/'.

Allow the client to add a prefix to all service names. Preferably as an optional value set when creating the channel.

This adds a complexity for _all clients_, which is really unfortunate. In my mind clients shouldn't need to know about this sort of thing. There are workarounds for Java that are a bit ugly, but functional. This is a bit of a touchy subject.

It would be desirable to have an optional parameter in the proto file that will trigger small client code generation for adding the prefix (maybe not optional if defined in the proto file)

This isn't bad, although I question how it would work in practice. It only works when there will be a single implementation of a service. If you think about something like our reflection and monitoring services, this sort of solution wouldn't function.

[dapengzhang0]

A possible workaround (without client adding prefix):
Server side: add a javax.servlet.Filter to modify grpc request's path to a proper contextPath + serveltPath and/or use RequestDispatcher to forward request to a proper webapp.
In case app1 and app2 both have GreetService, use additional headers to differentiate app1 and app2 (Client side is responsible to send the additional headers).

In case both apps will use grpc, and is what should be supported. This will happen:

app1 exposes a grpc called GreetService and we will be able to route it properly as it owns the root path. OK.
app1 exposes a grpc called InternalService and the servlet will route the request to app2. NOT_OK.
app2 will want to expose GreetService too!, but it will always be routed to app1. NOT_OK.
For grpc to work in a servlet container the proper way to support it IMHO is to:

Allow the client to add a prefix to all service names. Preferably as an optional value set when creating the channel.
Allow the server to be aware of that prefix.

[dapengzhang0]

@hofmanndavid Have you been actively making progress on it? Would you mind if I pick it up? Just in case a competing implementation may make you uncomfortable if you had something already.

hofmanndavid commented on Jun 11
Okay, Will try to get back when I have something. give me 2 weeks.

ejona86 commented on Jun 11
It would implement InternalServer and call ServerListener each inbound RPC. There's a handful of HTTP-isms that we've not split out of NettyServerHandler.onHeadersRead like we have with Http2ClientStreamTransportState, but that's not too big of a deal. For the while it could just be duplicated code.

[hofmanndavid]

@dapengzhang0 I am utterly sorry I could not make any progress in this part yet.

[hofmanndavid]

@dapengzhang0 I don't see how a ServletFilter will work as it works inside an already defined context path (e.g. /, /app1 or /app2)

Basically anything that does not involve the client adding the deployed application context path will involve a server-specific configuration.

[dapengzhang0]

@hofmanndavid By calling ServletContext.getContext(String uripath) you can get even another web application's servletContext as long as both apps are deployed in the same container.

// Filter forwarding /thisWebApp/* to /anotherWebApp
// public void doFilter(...) {
request.getServletContext()
    .getContext("/anotherWebApp")
    .getNameDispatcher("anotherWebAppServlet")
    .forward(request, response);

I don't see how a ServletFilter will work as it works inside an already defined context path (e.g. /, /app1 or /app2)

[hofmanndavid]

@dapengzhang0 I didn't know you can get other contexts on the same server. But still, in your example /thisWebApp/* and /anotherWebApp are deployed. Assuming we have a regular GreetService, clients will try to /GreetService and no filter will get the chance to get the request.

[dapengzhang0]

@hofmanndavid Yes, you need at least to deploy the filter at /GreetService. In the case both /app1 and /app2 have implementation logic of greeter service, the filter could forward requests to /app1 or /app2 depending additional custom header in the original request. If you can not deploy anything to /GreetService or /*, then of course you can not handle regular grpc requests.

[dapengzhang0]

@cs224 I'm currently working on this. I'd really like to hear from users, how would you like the API to look like.
For example, suppose you already have some grpc service classes, and you want a servlet to serve these grpc services, how do you expect the API I'm providing to initialize/register the servlet?

Could someone please give a status update on this topic? Thanks!

[RaphaelKellerAdc]

@dapengzhang0
We are currently testing your implementation using Wildfly 14.01.Final. We use a ServletContainerInitializer and CDI to find and register the grpc services.

/**
 * Adds url mapping (urlPattern) for all gRPC Services to {@link GrpcServerServlet}.
 */
public class GrpcServletContainerInitializer implements ServletContainerInitializer {

   @Override public void onStartup(Set<Class<?>> c, ServletContext ctx) throws ServletException {

      Instance<BindableService> grpcServices = CDI.current().select(BindableService.class);
      System.out.println("GrpcServletContainerInitializer");
      grpcServices.forEach(System.out::println);

      ctx.setAttribute("grpcServices", grpcServices);
      ServletRegistration.Dynamic servletRegistration = ctx.addServlet("grpcServlet",
            new GrpcServerServlet(grpcServices));
      servletRegistration.setAsyncSupported(true);

      for (BindableService service : grpcServices) {
         String urlPattern = "/" + service.bindService().getServiceDescriptor().getName() + "/*";
         System.out.println("Registering servlet url pattern: " + urlPattern);
         servletRegistration.addMapping(urlPattern);
      }
   }
}

This seems to work pretty well.
We also need to set our own interceptors in the server builder... As this functionality is not provided by the GrpcServlet class we did our own implementation of this - but I think this should be okay.

Can you tell us how the current progress of this project? Do you think the servlet implementation is going to be included in the grpc-java standard?

Thank you very much for your effort.

[dapengzhang0]

@RaphaelKellerAdc Thank you for your feedback! The current status of this project is (1) collecting feedback from potential users, (2) adding test code, (3) writing documentation. Once they are done, the servlet implementation will be part of grpc-java library in a separate package io.grpc.servlet. Your feedback will definitely speed up the progress.

For your usecase with interceptors, would it help to add a constructor of GrpcServlet with ServletServerBuilder as an argument?

  /**
   * Instantiate the servlet with the given serverBuilder.
   */
  public GrpcServlet(ServletServerBuilder serverBuilder) {
    servletAdapter = ServletAdapter.Factory.create(serverBuilder);
  }

[RaphaelKellerAdc]

@dapengzhang0 Thank you for the update. We will let you know if and when we have further insights.

>

For your usecase with interceptors, would it help to add a constructor of GrpcServlet with ServletServerBuilder as an argument?

  /**
   * Instantiate the servlet with the given serverBuilder.
   */
  public GrpcServlet(ServletServerBuilder serverBuilder) {
    servletAdapter = ServletAdapter.Factory.create(serverBuilder);
  }

Yes, this would help and should give enough control over the server building process.

[everflux]

Is grpc-web supported without having to use a special proxy as well?

[ejona86]

@everflux, this does not include grpc-web support.

[ehallander9591]

I have some underlying functionality that I would like support both a grpc req/rsp implementation as well as JAX-RS where the payloads could be json or encoded protobuf. This seems like an option which will allow both implementations to share the resources of the underlying subsystem.

I was glancing through the diffs and the examples, and so I expect that with this servlet helloworld deployed, the existing helloworld client should work without any changes?

[dapengzhang0]

@eamonnmcmanus The servlet helloworld deployed is serving grpc, so the existing helloworld client should work without any changes (Assuming the servlet is deployed to the right port and endpoint).

[lastcmaster]

Ok, I am just trying to test that an HelloWorld works on Wildfly, testing with 17.0.1.Final. And is seems to be working but I get this error when I deploy a simple servlet extending GrpcServlet:
Code:

@WebServlet(urlPatterns = {"/helloworld.Greeter/SayHello"}, asyncSupported = true)
public class HelloWorld extends GrpcServlet {
    private static final long serialVersionUID = 2879650933882294651L;

    public HelloWorld() {
        super(Collections.singletonList(new GreeterImpl()));
    }
}

Exception:

19:01:48,743 DEBUG [org.wildfly.extension.batch] (MSC service thread 1-3) Creating batch environment; ModuleClassLoader for Module "deployment.grpc-hello-world.war" from Service Module Loader
19:01:48,744 DEBUG [org.jboss.as.ee] (MSC service thread 1-6) Configuring component class: javax.servlet.jsp.jstl.tlv.PermittedTaglibsTLV named javax.servlet.jsp.jstl.tlv.PermittedTaglibsTLV
19:01:48,744 DEBUG [org.jboss.as.ee] (MSC service thread 1-6) Configuring component class: javax.servlet.jsp.jstl.tlv.ScriptFreeTLV named javax.servlet.jsp.jstl.tlv.ScriptFreeTLV
19:01:48,745 DEBUG [org.jboss.as.ee] (MSC service thread 1-6) Configuring component class: com.ismobile.test.war.hw.HelloWorld named com.ismobile.test.war.hw.HelloWorld
19:01:48,745 DEBUG [org.jboss.as.ee] (MSC service thread 1-6) Configuring component class: io.grpc.servlet.ServletAdapter$GrpcAsycListener named io.grpc.servlet.ServletAdapter$GrpcAsycListener
19:01:48,745 WARN  [org.jboss.as.ee] (MSC service thread 1-6) WFLYEE0007: Not installing optional component io.grpc.servlet.ServletAdapter$GrpcAsycListener due to an exception (enable DEBUG log level to see the cause)
19:01:48,745 DEBUG [org.jboss.as.ee] (MSC service thread 1-6) Not installing optional component io.grpc.servlet.ServletAdapter$GrpcAsycListener due to an exception: org.jboss.as.server.deployment.DeploymentUnitProcessingException: WFLYEE0048: Could not find default constructor for class io.grpc.servlet.ServletAdapter$GrpcAsycListener
    at org.jboss.as.ee.component.DefaultInterceptorConfigurator.configure(DefaultInterceptorConfigurator.java:92)
    at org.jboss.as.ee.component.deployers.EEModuleConfigurationProcessor.deploy(EEModuleConfigurationProcessor.java:92)
    at org.jboss.as.server.deployment.DeploymentUnitPhaseService.start(DeploymentUnitPhaseService.java:176)
    at org.jboss.msc.service.ServiceControllerImpl$StartTask.startService(ServiceControllerImpl.java:1737)
    at org.jboss.msc.service.ServiceControllerImpl$StartTask.execute(ServiceControllerImpl.java:1699)
    at org.jboss.msc.service.ServiceControllerImpl$ControllerTask.run(ServiceControllerImpl.java:1557)
    at org.jboss.threads.ContextClassLoaderSavingRunnable.run(ContextClassLoaderSavingRunnable.java:35)
    at org.jboss.threads.EnhancedQueueExecutor.safeRun(EnhancedQueueExecutor.java:1982)
    at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.doRunTask(EnhancedQueueExecutor.java:1486)
    at org.jboss.threads.EnhancedQueueExecutor$ThreadBody.run(EnhancedQueueExecutor.java:1363)
    at java.lang.Thread.run(Thread.java:748)

I have not dug deeper into it, as the client call works for both sync and async case. Using the example servlet from the tree, that inherits from HttpServlet does not generate this error.

Is this expected or does the class miss a default constructor?

[dapengzhang0]

@lastcmaster Thanks for trying it out. I don't see any essential difference between your servlet and the example servlet. Actually I'm trying to use an extension of GrpcServlet as you did in the example instead of the current example. I searched for the error message and seems it does not necessarily mean anything wrong.

I'm not familiar with WildFly. Did you config anything in jboss-web.xml?

[lastcmaster]

My jboss-web.xml only contains the content root element:
<context-root>/</context-root>

[swankjesse]

FWIW we were able to get it working in Jetty. Our servlet is pretty custom but it’s definitely possible.
https://github.com/cashapp/misk/blob/08b531b1bc9f1c3f551fe2849a10392284b87d15/misk/src/test/kotlin/misk/grpc/GrpcConnectivityTest.kt

The only trick was for duplex streaming we needed this option:

HttpConfiguration#isDelayDispatchUntilContent = false 

[wtlucy]

I've been doing some testing with #4738, on top of Open Liberty. Are there any recent updates for this, and is it likely that #4738 will make it into a release?

[rogeriob2br]

@hofmanndavid , @dapengzhang0 , @ejona86 : I was left with a doubt, if I keep the root path only for my gRPC implementations, and the contexts for HTTP1 implementations, will I encounter problems?

I am implementing tests using liberty, and websphere application server 9 on z / OS. Until next week I'll answer how we're doing.

[rogeriob2br]

I've been doing some testing with #4738, on top of Open Liberty. Are there any recent updates for this, and is it likely that #4738 will make it into a release?

Do you have any update?

[wtlucy]

@rogeriob2br the grpc maintainers haven't given any updates on this issue in some time. There shouldn't be any problem keeping "the root path only for my gRPC implementations". WAS 9 isn't going to support this at all, since it doesn't provide an HTTP/2 implementation. Liberty partially supports #4738, but full duplex streaming isn't working. From the OpenLiberty side, we're working on new features to directly support grpc - see https://github.com/OpenLiberty/open-liberty/issues/8637

[dapengzhang0]

I was focused on some other priorities. As there are more and more people interested in this feature, I will pickup it soon. Sorry for the delahy.

[dapengzhang0]

Liberty partially supports #4738, but full duplex streaming isn't working.

@wtlucy In what way full duplex streaming isn't working? Any error or hanging?

[dapengzhang0]

I was left with a doubt, if I keep the root path only for my gRPC implementations, and the contexts for HTTP1 implementations, will I encounter problems?

If your application server supports both HTTP/2 and HTTP1 traffic, then it should work for grpc client to grpc path and normal HTTP/1 client to other path.

[rogeriob2br]

@rogeriob2br the grpc maintainers haven't given any updates on this issue in some time. There shouldn't be any problem keeping "the root path only for my gRPC implementations". WAS 9 isn't going to support this at all, since it doesn't provide an HTTP/2 implementation. Liberty partially supports #4738, but full duplex streaming isn't working. From the OpenLiberty side, we're working on new features to directly support grpc - see OpenLiberty/open-liberty#8637

:(

[wtlucy]

@wtlucy In what way full duplex streaming isn't working? Any error or hanging?

@dapengzhang0 there's an error - but the problem is with Liberty's async behavior. We're working on a fix on that side.