Bidi Blocking Stub #10318

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

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

YifeiZhuang

not done yet, put what I have.

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

YifeiZhuang · 2023-07-19T17:40:21Z

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

+    if (!origWriteClosed && closedStatus == null) {
+      call.halfClose();
+    } else if (origWriteClosed) {
+      throw new IllegalStateException("halfClose cannot be called after stream terminated");


What should be the behaviour if closeStatus is non null?

the code looks like halfClose can not be called twice or called after cancel, but the error message seems the situation where the call is terminated from listener first, then calling halfClose is IlegalStateException.

If the listener closed the stream, but the client hadn't called halfClose or cancel, then we want to just ignore it.
It is a timing condition whether the halfClose happens before or after the stream is closed, the caller isn't going to do anything in response to the error and the stream is already in the desired state (closed); therefore it is better to do nothing than throw an error.

Maybe it can be simplified to:

if (writeClosed) { throw... } else if (getClosedStatus() == null) { call.halfClose(); } writeClosed = true;

Is it to prevent halfclose when call is already cancelled? Then this is still seems not correct. We would need an atomicInteger in both places.

Nice readability improvement. Done.

Why do you think that we would need an AtomicInteger?

Because I thought the API in this class should be thread safe - if user has one thread to call halfClose and the other thread to call cancel, then the code won't be able to handle the timing condition.
However, if thread safety is not a concern, (ClientCall itself is not thread safe as publicly documented), then there is no need for atomicInteger.

Also the getClosedStatus() seem can be removed. Because there is an issue:

t1: halfclose is called
t2: stream is closed and closedStatus=ok
t3: check closedSatus(L271)

then halfClose in t1 is lost.

If one thread calls halfClose and the other cancel then it is valid both for the halfClose to succeed or to throw an exception. The exception isn't thrown because it is causing the system problems, but rather because it likely indicates that there is a problem in the user's logic which they should look into.
If the client calls halfClose after the server sends close it won't cause a problem, it is just inefficient. In the example above, it is fine for halfClose not to be sent to the server because the server is by design no longer listening since it sent its own close.

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

ejona86

Only scratched the surface. I think I'm following along well enough. I think the comments are all small except the one about ThreadlessExecutor, but that one I also said I wasn't looking at more at the moment.

...interop-testing/src/generated/debug/grpc/io/grpc/testing/integration/MetricsServiceGrpc.java

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

alts/src/generated/main/grpc/io/grpc/alts/internal/HandshakerServiceGrpc.java

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

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

YifeiZhuang · 2023-08-02T23:52:21Z

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

+    if (!origWriteClosed && closedStatus == null) {
+      call.halfClose();
+    } else if (origWriteClosed) {
+      throw new IllegalStateException("halfClose cannot be called after stream terminated");


Maybe it can be simplified to:

if (writeClosed) { throw... } else if (getClosedStatus() == null) { call.halfClose(); } writeClosed = true;

Is it to prevent halfclose when call is already cancelled? Then this is still seems not correct. We would need an atomicInteger in both places.

stub/src/test/java/io/grpc/stub/BlockingClientCallTest.java

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

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

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

The generated method is not ExperimentalApi.

I think that I misunderstood. I added the ExperimentalApi on ClientCalls and BlockingClientCall in our code. When we talked about it a long time ago I thought you were against putting the experimental annotation in the generated code.

Added experimental annotation to generated code.

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

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

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

This is still an open item.

We had put it in the ClientCalls method that this calls that we would rethrow it if we got interrupted while writing a request. Should we be handling it there and not rethrowing it?

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

larry-safran

Ready for rereview

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

larry-safran · 2024-09-16T21:58:02Z

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>


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

Added some 'final' to class definitions.

…est reliability.

Added some 'final' to class definitions.

…est reliability.

…king

This reverts commit 3c63771.

ejona86

If we can remove InterruptedException later and not break people (no error like "catching exception not thrown in try"), then we just need to experimental API annotation on the generated code before shipping.

ejona86 · 2024-10-17T22:00:54Z

compiler/src/java_plugin/cpp/java_generator.cpp

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


The generated method is not ExperimentalApi.

ejona86 · 2024-10-17T22:01:09Z

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"


This is still an open item.

ejona86 · 2024-10-17T22:05:53Z

FWIW, I do have a really hard time understanding what the state of the code was when I last reviewed, so that I can see the changes.

larry-safran · 2024-10-17T22:58:47Z

Sorry about making such a mess with the rebase. I'll wait until you're ready to approve before trying to rebase again.

ejona86 · 2024-10-18T22:09:04Z

Sorry about making such a mess with the rebase. I'll wait until you're ready to approve before trying to rebase again.

It was less that you did the rebase and more that I can't tell where I should start reading. If you had squashed all the commits before the rebase and fixes were on top, I'd have actually been fine. Right now the first commit is "Fix a bunch of things" so I get lost immediately.

I see now that if I keep looking at commits, they make sense again, so the first commit just had the wrong message. But also the commits are repeated a second time. Looks like it was just a bad rebase.

larry-safran · 2024-10-19T00:47:22Z

I did do a squash before the rebase, that's why I called it "Fix a bunch of things". I see that adding "Squashed" would have been a good idea.

…

On Fri, Oct 18, 2024 at 3:09 PM Eric Anderson ***@***.***> wrote: Sorry about making such a mess with the rebase. I'll wait until you're ready to approve before trying to rebase again. It was less that you did the rebase and more that I can't tell where I should start reading. If you had squashed all the commits before the rebase and fixes were on top, I'd have actually been fine. Right now the first commit is "Fix a bunch of things" so I get lost immediately. I see now that if I keep looking at commits, they make sense again, so the first commit just had the wrong message. But also the commits are repeated a second time. Looks like it was just a bad rebase. — Reply to this email directly, view it on GitHub <#10318 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AZQMCXQZKNRYNUBD3ZEHN2TZ4GBJPAVCNFSM6AAAAAAZWJRQKWVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDIMRTGMYDONBRGY> . You are receiving this because you commented.Message ID: ***@***.***>

ejona86 · 2024-10-23T20:35:14Z

that's why I called it "Fix a bunch of
things". I see that adding "Squashed" would have been a good idea.

A better name would have been "Bidi Blocking Stub" or similar, since it had all the implementation in it.

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

YifeiZhuang reviewed Jul 19, 2023

View reviewed changes

ejona86 reviewed Aug 3, 2023

View reviewed changes

YifeiZhuang reviewed Aug 3, 2023

View reviewed changes

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

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

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

ejona86 reviewed Dec 19, 2023

View reviewed changes

ejona86 reviewed Feb 15, 2024

View reviewed changes

larry-safran commented Feb 16, 2024

View reviewed changes

ejona86 reviewed Mar 29, 2024

View reviewed changes

ejona86 mentioned this pull request Jun 20, 2024

@CanIgnoreReturnValue for blocking stub methods returning Empty #11296

Open

larry-safran force-pushed the blocking branch from 244be52 to 3832896 Compare September 13, 2024 23:51

larry-safran commented Sep 16, 2024

View reviewed changes

larry-safran added 3 commits September 18, 2024 17:21

Fix a bunch of things

a0b0ddf

Restore things messed up by rebase

75f956d

Fixed hang from not getting interrupted.

90caf39

Added some 'final' to class definitions.

larry-safran force-pushed the blocking branch from 3832896 to 90caf39 Compare September 19, 2024 00:26

larry-safran added 7 commits September 18, 2024 17:52

Generated service with blocking stream methods.

5f760bd

Reverse order of operations in BlockingClientCall.cancel to improve t…

3238a3b

…est reliability.

Fix previously broken tests

abb25cb

Fix a bunch of things

7c46bed

Restore things messed up by rebase

3fb5106

Fixed hang from not getting interrupted.

7248099

Added some 'final' to class definitions.

Generated service with blocking stream methods.

3c63771

larry-safran added 3 commits September 24, 2024 15:27

Reverse order of operations in BlockingClientCall.cancel to improve t…

1f33acf

…est reliability.

Merge remote-tracking branch 'refs/remotes/origin/blocking' into bloc…

4402539

…king

Revert "Generated service with blocking stream methods."

663480b

This reverts commit 3c63771.

ejona86 reviewed Oct 17, 2024

View reviewed changes

Add ExperimentalApi to new blocking BlockingClientCall generated methods

2cead63

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 Loading

YifeiZhuang 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

ejona86 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

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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejona86 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ejona86 commented Oct 17, 2024

larry-safran commented Oct 17, 2024

ejona86 commented Oct 18, 2024

larry-safran commented Oct 19, 2024 via email

ejona86 commented Oct 23, 2024

larry-safran commented Jun 28, 2023 •

edited

Loading