servlet: Implement gRPC server as a Servlet

[dapengzhang0]

The goal is as described in #1621 to add the ability to run a gRPC server as a Servlet on any web container with the Servlet 4.0 support and HTTP/2 enabled.

cc @vlsinitsyn @cs224 @jnehlmeier @hofmanndavid

cc @meltsufin for high level design

API

This PR provides the following API:

An adapter

/**
 * An adapter that transforms {@link HttpServletRequest} into gRPC request and lets a gRPC server
 * process it, and transforms the gRPC response into {@link HttpServletResponse}. An adapter can be
 * instantiated by {@link ServletServerBuilder#buildServletAdapter()}.
 *
 * <p>In a servlet, calling {@link #doPost(HttpServletRequest, HttpServletResponse)} inside {@link
 * javax.servlet.http.HttpServlet#doPost(HttpServletRequest, HttpServletResponse)} makes the servlet
 * backed by the gRPC server associated with the adapter. The servlet must support Asynchronous
 * Processing and must be deployed to a container that supports servlet 4.0 and enables HTTP/2.
 */
public final class ServletAdapter {
   /**
   * Call this method inside {@link javax.servlet.http.HttpServlet#doGet(HttpServletRequest,
   * HttpServletResponse)} to serve gRPC POST request.
   */
  public doPost(HttpServletRequest, HttpServletResponse);

and a grpc server builder

/** Builder to build a gRPC server that can run as a servlet. */
public final class io.grpc.servlet.ServletServerBuilder extends ServerBuilder

  /**
   * <p>Users should not call this method directly. Instead users should call
   * {@link #buildServletAdapter()} which internally will call {@code build()} and {@code start()}
   * appropriately.
   * @throws IllegalStateException if this method is called by users directly
   */
  @Override
  public Server build();

  /** Creates a {@link ServletAdapter}.*/
  public ServletAdapter buildServletAdapter();

and a servlet impl

public class GrpcServlet extends HttpServlet {
  public GrpcServlet(List<BindableService> grpcServices);
}

---

A ServletAdapter instance will be backing one or more gRPC services with a ServletServerBuilder

ServletAdapter servletAdapter = 
    new ServletServerBuilder().addService(new HelloServiceImpl()).buildServletAdapter();

---

A servlet powering the gRPC services will be as simple as either

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

  @Override
  protected void doPost(HttpServletRequest req, HttpServletResponse resp) {
    servletAdapter.doPost(req, resp);  
  }
}

or

@WebServlet(asyncSupported = true)
public class HelloServlet extends GrpcServlet {
}

Alternative API

Hello World example

See examples/example-servlet/src/main/java/io/grpc/servlet/examples/helloworld/

Implementation

Aside from trivial API pluming, what the impl actually does is (in doPost() method)

• start an AsyncContext
• set WriteListener and ReadListener for the ServletOutputStream and ServletInputStream
• create and pass a new instance of io.grpc.servlet.ServletServerStream to ServerTransportListener.streamCreated(serverStream, method, headers)
• This ServletServerStream calls stream.transportState().inboundDataReceived() in the ReadListener.onDataAvailable() callback
• The ServletServerStream holds a WritableBufferAllocator for the outbound data that will write to the ServletOutputStream, and uses an atomic ref of ServletServerStream.WriteState to coordinate with the WriteListener.onWritePossible() callback.
• The ServletServerStream.TransportState.runOnTransportThread() method uses a SerializingExecutor(directExecutor())

Test result

We tested it with a servlet backed by the InteropTest gRPC service TestServiceImp, and an ordinary gRPC InteropTest client, for the test cases EMPTY_UNARY, LARGE_UNARY, CLIENT_STREAMING, SERVER_STREAMING, and PING_PONG. The following are the test results of some of the web container vendors

Jetty 10.0-SNAPSHOT

All tests passed! Jetty looks fully compliant with the Servlet 4.0 spec and HTTP/2 protocol. :100: @sbordet

---

GlassFish 5.0 - Web Profile

All tests passed with minor workaround. An issue of GlassFish is filed, cc @mattgill98 for help:

• Async servlet failed to send large data frames completely when HTTP/2 trailers are present

---

Tomcat 9.0.10

Non-blocking servlet I/O (ReadListener/WriteListener) practically can not work in full-duplex case. Only EMPTY_UNARY test passed. (Update: Filed multiple bugs to Tomcat: cc @markt-asf for help)

• Async servlet over HTTP/2 setReadListener does not work if post request data arrives much later than headers - fixed 9.0.11 onwards
• Async servlet over HTTP/2 WriteListener does not work because onWritePossible is never called back again - fixed 9.0.11 onwards
• Async servlet over HTTP/2 large write data corrupted - fixed 9.0.12 onwards
• Async servlet over HTTP/2 non-blocking write does not work if client sets custom SETTINGS_INITIAL_WINDOW_SIZE - fixed 9.0.12 onwards
• Async servlet over HTTP/2 response.flushBuffer() intermittently fails - Not a bug, the async flush feature is not specified by the Servlet 4.0 spec; but a patch to support it is still added by Tomcat. Really appreciate it.
• Async servlet over HTTP/2 on embedded server sporadically timeout forever when reading near end of request - fixed 9.0.14 onwards

(Update: Tomcat 9.0.x trunk

All tests passed! :100:)

---

Undertow 2.0.11.Final/WildFly-14.0.0.Beta1

Not able to test. Simply can not make the servlet container send out trailers for plain servlet. cc @ctomc @stuartwdouglas for help (Update: Identified as undertow bug

• HTTP/2 does not honour the trailer supplier attachment - fixed

)

(Update: Undertow 2.0.12.Final

All tests passed! :100:)

[stuartwdouglas]

Trailers should be supported. Is there an easy way for me to test this so I can see what is wrong?

[dapengzhang0]

@stuartwdouglas The StackOverflow question has provided a simple servlet, I couldn't even make that work. I might have missed some configuration. If you can successfully run that servlet and verify that client side receives the trailers, just let me know the steps I might have missed. Really appreciate it. On my machine, I can verify Glassfish, Tomcat and Jetty all send trailers, but Undertow/Wildfly does not.

[stuartwdouglas]

This should be fixed in Undertow 2.0.12.Final, which will be released shortly.

[dapengzhang0]

Thanks a lot @stuartwdouglas , I will test on it once it's released. Right now it is not straight forward to run the InteropTest for this PR, I'm trying to make the steps easier.

[sbordet]

@dapengzhang0 can you try Jetty 9.4.11? It should have similar support for 4.0, trailers, etc. that Jetty 10 has.

[dapengzhang0]

@sbordet I tried Jetty 9.4.11, it did not work

java.lang.NoSuchMethodError: javax.servlet.http.HttpServletResponse.setTrailerFields

It seems it does not support servlet 4.0. Actually its release note didn't claim it supports servlet 4.0. And it's pom

<dependencyManagement>
<dependencies>
<dependency>
<groupId>javax.servlet</groupId>
<artifactId>javax.servlet-api</artifactId>
<version>3.1.0</version>
</dependency>
...

[sbordet]

@dapengzhang0 thanks for trying. Yes, with 9.4.x you need to downcast to Jetty specific classes to get the trailer methods. Never mind 9.4.x, and happy that Jetty 10.0.x works well for this case.

[dapengzhang0]

There's a lot here, and much of it has obvious things that need to be cleaned up before a real review. What did you want me to look at for the initial review?

@ejona86 aside from impl details, for the initial review, is there anything fundamentally in a wrong approach, or anything important is missing (such as runOnTransportThread() I had missed), or anything I must fix before adding new stuff?

[dapengzhang0]

Added InteropTest and TransportTest with embedded Undertow servlet container. Rebased. @ejona86 PTAL

[ehallander9591]

It seems like this is moving closer to being included. I will keep monitoring it.

[dapengzhang0]

@ehallander9591 Thank you for following the progress. I'll still working on it. Will send out updated PR in a week or so.

[dapengzhang0]

@creamsoup Would you like to be a second reviewer, on AbstractStream part at least?

[creamsoup]

@dapengzhang0, sure.

[dapengzhang0]

@creamsoup

• WriteListener#onWritePossible just calls the doWrite (similar to SynchronizedContext#drain).

• any writes in Sink (header, data frame, trailers etc) need to be queued (SynchronizedContext#executeLater). after queue the write, it must call doWrite immediately. Those two steps are similar to SynchronizedContext#execute.

• doWrite checks Reentrancy to make sure there is only one write just like SynchronizedContext#drain. the later doWrite returns immediately.

This is a great idea. However, on a second thought of the above approach, I found that it may cause the container to be in an illegal state in the following scenario:

(1) After some doWrite(), outputStream.isReady() returns false. An onWritePossible() callback will be called by the container later. (2) Then the sink tries to write some data.There is a race between Sink's doWirte and onWritePossible(). Suppose Sink's write wins the race. The sink will check outputStream.isReady() before the already scheduled/promised onWritePossible() is called. This violate the contract, the behavior is undefined, and may cause problem for some containers.

To solve this problem, we may still need something like writeState.stillWirtePossible, which adds complexity.

Also, we can not completely reuse SynchronizedContext or SerializingExecutor because we can not drain() all the buffered things. Instead, we need drainWhileReady(). This also adds complexity, and hard to prove the implementation's correctness.

[ejona86]

I remember vaguely looking at the write flow before. I agree it is complicated. Since the writes are async, I think we should just use a lock for the write path, including writing to the async output stream. It seems that would greatly simplify the code.

[dapengzhang0]

@ejona86 Current write path implementation is so difficult to see the correctness. Will test out using a lock for the write path. Will update the PR if it works for all the three major containers. If any of containers has an issue, I will update the PR once the issue is fixed on the container side (Must guarantee the impl is well tested on three containers).

[dapengzhang0]

Inspired by @creamsoup , I found a simple and clean solution, reusing SerializingExecutor or SynchronizationContext, without lock, without other AtomicReference:

Executor wirteExecutor = new SerializingExecutor(directExecutor());// SyncCtx also works
boolean isWriteReady;
Queue< WritableBuffer> wirteQueue = new ArrayDeque();

void onWritePossible() {
  writeExecutor.execute(
      () -> {
        assert !isWriteReady;
        isWriteReady = outputStream.isReady(); 
        // the above is the ONLY place isWriteReady could flip from false to true

        while (isWriteReady && !writQueue.isEmpty()) {
          WritableBuffer buffer = writeQueue.poll();   
          outputStream.wirte(writeQueue.poll().readableBytes());
          isWriteReady = outputStream.isReady();
          // if the above returns false, another onWritePossible() will be called
          // by the container *later*,
        }
      });
}

// The difference from @creamsoup's approach is that writeFrame here never calls drain()
void writeFrame(WritableBuffer buffer) {
  writeExecutor.execute(
    () -> {
      if (isWriteReady) {
        assert writeQueue.isEmpty();
        outputStream.wirte(buffer.readableBytes());
        isWriteReady = outputStream.isReady();
        // if the above returns false, onWritePossible() will be called
        // by the container *later*
      } else {
        writeQueue.offer(buffer);
      }
    })
}

Update: This approach also has a subtle bug.

[creamsoup]

@dapengzhang0, that looks very concise and easy to follow. does it work on all 3 servlets?

[hypnoce]

Hi all, is there a plan to get this reviewed and/or merged ? I'm considering forking grpc locally to build one supporting servlet unless this PR makes it through. Thanks

[lprimak]

Is this still maintained?

[sanjaypujare]

Is this still maintained?

@dapengzhang0 are you planning to drive this to completion?

[lprimak]

Trying to use this in Payara, and getting all sorts of intermittent errors. Doesn't look like this works correctly currently Looks to me like this is abandoned. What a shame.

[dapengzhang0]

Trying to use this in Payara, and getting all sorts of intermittent errors.

@lprimak, I tested GlassFish 5.0/Grizzly, it didn't pass all the interop tests like Jetty/UnderTow/Tomcat. Seems there are some HTTP/2 & aysnc-servlet issues in Grizzly. Payara is also based on Grizzly kernel so there could be the same problems. I didn't have time to identify the Grizzly bugs. Sorry for being inactive for a long time.

[lprimak]

@dapengzhang0 Thanks, but are you maintaining this branch? Will it ever be merged? It's not compatible with the latest gRPC core even.

[lprimak]

I tested GlassFish 5.0/Grizzly, it didn't pass all the interop tests like Jetty/UnderTow/Tomcat. Seems there are some HTTP/2 & aysnc-servlet

Looks like these issues have been fixed in Payara in 2018 @dapengzhang0

[lprimak]

Can you at least update this PR to fix https://github.com/grpc/grpc-java/pull/4738#pullrequestreview-617677307 and have it working on the lastes gRPC-Java please? Thank you!

[rufreakde]

@dapengzhang0 is this worked on? Looking forward to this! <3

[dapengzhang0]

A newer version of this work is #8596, although there is no update on it for a long time too. Close this PR.

[hutchig]

https://github.com/grpc/grpc-java/pull/8596 is now merged in https://github.com/grpc/grpc-java/releases/tag/v1.53.0