Skip to content
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

servlet: Implement gRPC server as a Servlet #4738

Closed
wants to merge 32 commits into from

Conversation

dapengzhang0
Copy link
Member

@dapengzhang0 dapengzhang0 commented Aug 4, 2018

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. 💯
@sbordet


GlassFish 5.0 - Web Profile

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


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)

(Update: Tomcat 9.0.x trunk

All tests passed! 💯)


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

)

(Update: Undertow 2.0.12.Final

All tests passed! 💯)

@dapengzhang0 dapengzhang0 force-pushed the servletasync branch 7 times, most recently from d967b77 to 47b7090 Compare August 7, 2018 00:57
@stuartwdouglas
Copy link

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

@dapengzhang0
Copy link
Member Author

@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
Copy link

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

@dapengzhang0
Copy link
Member Author

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
Copy link

sbordet commented Aug 8, 2018

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

@dapengzhang0
Copy link
Member Author

@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
Copy link

sbordet commented Aug 9, 2018

@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 dapengzhang0 force-pushed the servletasync branch 2 times, most recently from 61b001f to 09c60d8 Compare August 10, 2018 18:49
Copy link
Member

@ejona86 ejona86 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

servlet/src/main/java/io/grpc/servlet/ServerBuilder.java Outdated Show resolved Hide resolved
core/src/main/java/io/grpc/internal/ServerImpl.java Outdated Show resolved Hide resolved
servlet/src/main/java/io/grpc/servlet/ServerBuilder.java Outdated Show resolved Hide resolved
servlet/src/main/java/io/grpc/servlet/ServerStream.java Outdated Show resolved Hide resolved
settings.gradle Outdated Show resolved Hide resolved
servlet/src/main/java/io/grpc/servlet/ServletAdapter.java Outdated Show resolved Hide resolved
servlet/src/main/java/io/grpc/servlet/ServletAdapter.java Outdated Show resolved Hide resolved
@dapengzhang0
Copy link
Member Author

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 dapengzhang0 force-pushed the servletasync branch 6 times, most recently from 1ebbc6e to 6667208 Compare August 17, 2018 17:46
@dapengzhang0
Copy link
Member Author

@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
Copy link
Member Author

dapengzhang0 commented Aug 8, 2019

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
Copy link
Contributor

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

transportListener.transportTerminated();
}

private static final class GrpcAsycListener implements AsyncListener {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo? Asyc -> Async

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. Thanks. I also noticed that just had not a chance to fix yet.

}

@Override
protected void startServer(ServerBuilder<?> builer) {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

protected void startServer(ServerBuilder<?> builder)

change builer to builder on this line and the next?

@hypnoce
Copy link
Contributor

hypnoce commented Feb 27, 2021

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

Copy link

@MattGill98 MattGill98 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Firstly, really nice work on this feature - I think it's a brilliantly useful idea. I've been doing some testing and one bug came up for me that I wanted to mention!

this.logId = logId;
this.asyncCtx = asyncCtx;
this.resp = (HttpServletResponse) asyncCtx.getResponse();
resp.getOutputStream().setWriteListener(new GrpcWriteListener());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When testing against Payara server, this class seemingly fails to completely initialise unless the writer field is initialised before the write listener on this line. When swapped I get NPEs in the OutputBuffer, but I'm willing to assume this could just be an expected Grizzly bug. I do, however, at least get the expected communication between the client and server.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In c903d75#diff-61714cd00cc4b70a569fd5ac7ac6919bcc9e2e76b6c0384ffbbeda9587c5ff93 of #8596, I swapped the assignment of the writer field. Thanks for catching this.

@lprimak
Copy link

lprimak commented May 13, 2021

Is this still maintained?

@sanjaypujare
Copy link
Contributor

Is this still maintained?

@dapengzhang0 are you planning to drive this to completion?

@lprimak
Copy link

lprimak commented May 13, 2021

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
Copy link
Member Author

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
Copy link

lprimak commented May 13, 2021

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

@lprimak
Copy link

lprimak commented May 16, 2021

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
Copy link

lprimak commented May 16, 2021

Can you at least update this PR to fix #4738 (review) and have it working on the lastes gRPC-Java please?
Thank you!

@rufreakde
Copy link

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

@dapengzhang0
Copy link
Member Author

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

@hutchig
Copy link

hutchig commented Mar 17, 2023

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 16, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.