Bidi Blocking Stub #10318

larry-safran · 2023-06-28T00:20:38Z

Created a BlockingClientCall class that does blocking streams for all 3 streaming types.

…ork. Multiple threads blocking on a single ClientCall was hanging.

…es. Fix multithreading logic in ThreadExecutor.

stub/src/main/java/io/grpc/stub/BlockingClientCall.java

…alue of stub on blocking server streaming calls Also in generator put a space between request and response types in the generic definition and use java.lang.String everywhere as has already been done in master

… and move the logic out of the generated file into the server streaming method

…into blocking

…ThreadlessExecutor class for multithreading.

stub/src/main/java/io/grpc/stub/ClientCalls.java

stub/src/main/java/io/grpc/stub/BlockingClientCall.java

stub/src/main/java/io/grpc/stub/ClientCalls.java

Co-authored-by: Eric Anderson <ejona@google.com>

…upport to Lock & Condition

stub/src/main/java/io/grpc/stub/BlockingClientCall.java

ejona86 · 2024-02-15T00:59:54Z

stub/src/main/java/io/grpc/stub/ClientCalls.java

+    private static final Logger log =
+        Logger.getLogger(ThreadSafeThreadlessExecutor.class.getName());
+
+    private Lock waiterLock = new java.util.concurrent.locks.ReentrantLock();


final here and the condition. Since it is multi-threading, the final is particularly helpful.

This still needs final

stub/src/main/java/io/grpc/stub/ClientCalls.java

stub/src/main/java/io/grpc/stub/BlockingClientCall.java

ejona86 · 2024-02-15T01:31:08Z

compiler/src/java_plugin/cpp/java_generator.cpp

+        if (client_streaming && server_streaming) {
+          p->Print(
+              *vars,
+              "$BlockingClientCall$<$input_type$, $output_type$>\n"


Let's go ahead and mark these @ExperimentalApi for at least one release? The code here is pretty tight. The main thing I could see that could change is BlockingClientCall<?, RespT> becoming BlockingClientCall<ReqT, RespT>, because that seems the only choice we have.

Although maybe we could also remove the throws InterruptedException from server streaming, since we "know" it won't block. Dunno if that is worth it.

Done. Created #10918

…atusException for server streaming and regenerate stubs.

larry-safran

Need a decision on whether should pass ClientCall instead of channel, method and call options. Then need to update generated code.

larry-safran · 2024-02-15T23:07:45Z

compiler/src/java_plugin/cpp/java_generator.cpp

+        if (client_streaming && server_streaming) {
+          p->Print(
+              *vars,
+              "$BlockingClientCall$<$input_type$, $output_type$>\n"


Done. Created #10918

stub/src/main/java/io/grpc/stub/BlockingClientCall.java

stub/src/main/java/io/grpc/stub/ClientCalls.java

larry-safran · 2024-02-16T00:44:09Z

stub/src/main/java/io/grpc/stub/ClientCalls.java

+    private static final Logger log =
+        Logger.getLogger(ThreadSafeThreadlessExecutor.class.getName());
+
+    private Lock waiterLock = new java.util.concurrent.locks.ReentrantLock();


stub/src/main/java/io/grpc/stub/ClientCalls.java

stub/src/main/java/io/grpc/stub/BlockingClientCall.java

…mingCall

…tusException

ejona86 · 2024-03-29T16:09:26Z

stub/src/main/java/io/grpc/stub/ClientCalls.java

@@ -869,8 +857,8 @@ static final class ThreadSafeThreadlessExecutor extends ConcurrentLinkedQueue<Ru
    private static final Logger log =
        Logger.getLogger(ThreadSafeThreadlessExecutor.class.getName());

-    private Lock waiterLock = new java.util.concurrent.locks.ReentrantLock();
-    private Condition waiterCondition = waiterLock.newCondition();
+    private Lock waiterLock = new ReentrantLock();


This still needs final

ejona86 · 2024-03-29T16:09:43Z

stub/src/main/java/io/grpc/stub/ClientCalls.java

+    private static final Logger log =
+        Logger.getLogger(ThreadSafeThreadlessExecutor.class.getName());
+
+    private Lock waiterLock = new java.util.concurrent.locks.ReentrantLock();


This still needs final

ejona86 · 2024-03-29T16:12:45Z

stub/src/main/java/io/grpc/stub/ClientCalls.java

@@ -340,7 +413,7 @@ private abstract static class StartableListener<T> extends ClientCall.Listener<T
    abstract void onStart();
  }

-  private static final class CallToStreamObserverAdapter<ReqT>
+  private static class CallToStreamObserverAdapter<ReqT>


As discussed offline, the static and final are separate. This should still be final.

ejona86 · 2024-03-29T16:16:00Z

compiler/src/java_plugin/cpp/java_generator.cpp

+          p->Print(
+              *vars,
+              "$BlockingClientCall$<?, $output_type$>\n"
+              "    $lower_method_name$($input_type$ request) throws java.lang.InterruptedException,\n"


We should discuss whether we remove InterruptedException here. (We know it won't throw.)

ejona86 · 2024-03-29T16:17:44Z

stub/src/main/java/io/grpc/stub/ClientCalls.java

+   * @throws StatusException if the write to the server failed
+   */
+  @ExperimentalApi("https://github.com/grpc/grpc-java/issues/10918")
+  public static <ReqT, RespT> BlockingClientCall<?, RespT> blockingV2ServerStreamingCall(


To allow us to change the ? to ReqT in the generated code if we find it works better, we may want the ReqT present here but use ? in the generated code. We can talk about that.

larry-safran added 23 commits April 14, 2023 13:06

checkpoint

138c1ad

Checkpoint

9b2ab2a

Initial pass complete

fd26f23

Change blockingWriteOrRead to be either/or rather than and/or.

515c7c4

Update javadoc and checking for write legality

4e63935

Feedback from API review

09095ae

Generate blocking bidi stream method

7d87f74

Add generated method for bidi streaming

2fe79a9

Restore BlockingServerStream's QueueingListener to the original.

b2de4c7

Fix tests and formatting

4f9ca2d

Implement example

70a1b1b

example fixes

7fdb53a

Fixed while condition

d380b7d

Checkpoint

4dedccd

Updated examples and removed the writeOrRead that was confusing.

95b6b2f

Update interface to reflect latest conversations and make all tests w…

713d74a

…ork. Multiple threads blocking on a single ClientCall was hanging.

Updates based upon design review.

60af084

Fix test case.

3bd8b8c

Remove waitForReady.

d747b82

fix halfClose.

1b63ada

add padding to messages so that flow control actually happens on writ…

7beeaf5

…es. Fix multithreading logic in ThreadExecutor.

Fix style errors

d3a89d0

Merge branch 'master' into blocking

76f1892

larry-safran marked this pull request as ready for review June 28, 2023 00:33

larry-safran requested review from ejona86 and YifeiZhuang June 28, 2023 00:33

larry-safran added 4 commits June 29, 2023 17:04

Add comment

5593196

Fix cancel test

09226c8

Rename BlockingBiDiStream.java to BlockingClientCall

ef8d28a

Improve test reporting and increase delay amount to reduce flakes.

5f9d701

ejona86 reviewed Aug 3, 2023

View reviewed changes

stub/src/main/java/io/grpc/stub/BlockingClientCall.java Outdated Show resolved Hide resolved

ejona86 mentioned this pull request Aug 16, 2023

server-side streaming must call hasNext() before cancel #10490

Closed

larry-safran added 7 commits August 30, 2023 18:24

Changed the generator to use '?' instead of request type for return v…

d2dec86

…alue of stub on blocking server streaming calls Also in generator put a space between request and response types in the generic definition and use java.lang.String everywhere as has already been done in master

Add methods to ClientCalls for both blocking client and server stream…

2104a0f

… and move the logic out of the generated file into the server streaming method

Merge branch 'master' into blocking

6c06c90

Fix the logic so that onReady is actually handled correctly

c08eb90

Merge branch 'blocking' of https://github.com/larry-safran/grpc-java …

75d894e

…into blocking

Fix listener handling of onReady

6921d6a

Remove generics from the listener.

7a62a5a

ejona86 changed the title ~~Blocking~~ Bidi Blocking Stub Sep 21, 2023

Respond to code review comments

0252972

larry-safran requested a review from YifeiZhuang October 12, 2023 19:11

sanjaypujare mentioned this pull request Oct 25, 2023

Tracking Issue for Testing utilities being Experimental. #1791

Open

Rsstore ThreadlessExecutor to the way it was and use a new ThreadSafe…

ee8d7d8

…ThreadlessExecutor class for multithreading.

ejona86 reviewed Dec 19, 2023

View reviewed changes

larry-safran and others added 5 commits January 8, 2024 16:54

Update stub/src/main/java/io/grpc/stub/ClientCalls.java

c245f75

Co-authored-by: Eric Anderson <ejona@google.com>

Change locking logic in ThreadSafeThreadlessExecutor from using LockS…

5c70deb

…upport to Lock & Condition

Update javadoc to make it clear that flow control is a network thing.

7cd0df6

Eliminate synchronized from waitAndDrain, just using waiterLock.

78d9ce0

Cleanup with Eric

0ac6ad6

ejona86 reviewed Feb 15, 2024

View reviewed changes

Respond to Review comments. Need to update generated code to throw St…

61ca3c8

…atusException for server streaming and regenerate stubs.

larry-safran commented Feb 16, 2024

View reviewed changes

larry-safran added 5 commits February 16, 2024 14:35

Add StatusException to the throws list of blockingV2ServerStreamingCall

d379393

Updated generated code with StatusException for blockingV2ServerStrea…

ed3809f

…mingCall

Add missing generated classes

53a0766

change StatusRuntimeException to StatusException in catch blocks

05151ff

Updated examples to handle blockingV2ServerStreamingCall throwing Sta…

244be52

…tusException

ejona86 reviewed Mar 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bidi Blocking Stub #10318

Bidi Blocking Stub #10318

larry-safran commented Jun 28, 2023 •

edited

ejona86 Feb 15, 2024

larry-safran Feb 16, 2024

ejona86 Mar 29, 2024

ejona86 Feb 15, 2024

larry-safran Feb 15, 2024

larry-safran left a comment

larry-safran Feb 15, 2024

larry-safran Feb 16, 2024

ejona86 Mar 29, 2024

ejona86 Mar 29, 2024

ejona86 Mar 29, 2024

ejona86 Mar 29, 2024

ejona86 Mar 29, 2024

Bidi Blocking Stub #10318

Are you sure you want to change the base?

Bidi Blocking Stub #10318

Conversation

larry-safran commented Jun 28, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larry-safran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larry-safran commented Jun 28, 2023 •

edited