Bidi Blocking Stub

[larry-safran]

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

[YifeiZhuang]

not done yet, put what I have.

[YifeiZhuang]

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.

[larry-safran]

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.

[YifeiZhuang]

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.

[larry-safran]

Nice readability improvement. Done.

[larry-safran]

Why do you think that we would need an AtomicInteger?

[YifeiZhuang]

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.

[larry-safran]

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.

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

[YifeiZhuang]

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.

[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]

Ready for rereview

[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]

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]

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

[ejona86]

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]

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]

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.