Add disableAutoRequest method that disables all automatic inbound flow-control requests

[DRayX]

The default behavior of requesting initial messages is applied even if disableAutoInboundFlowControl is called. ServerCalls disables all automatic flow control which is much more useful in case the user can't handle incoming messages until some time after the call has started. This change creates a new StartableListener that has an onStart method that is invoked when the call is started which makes initial requests if necessary.

See #6806

[linux-foundation-easycla]

The committers are authorized under a signed CLA.

• ✅ DRayX (87bb38a, 86752b0, 7029ad7, 4fd1176)

[sanjaypujare]

@DRayX can you take care of CLA authorization?

[DRayX]

Pull request looks good to go now. Note that I changed the approach based on discussion with @ejona86 in #6806.

[ejona86]

Copying this from #6806. @DRayX said:

There is one interesting change that is worth noting. On the client-side, for unary and server-streaming calls (unary request), the ClientCallStreamObserver isn't returned from the stub, but request can't be used in ClientResponseObserver.beforeStart (since ClientCallImpl.request checks that the call is started). Thus, the ClientResponseObserver implementation needs to expose some mechanism to make the initial request. I updated ManualFlowControlClient example to use this pattern (even though it uses bidi-streaming).

That's an important realization. I wonder if we should allow request()s to be "queued" ("counted") during beforeStart() and then "drain" them after calling start(). The docs for that will be a bit weird, especially if we don't make it thread-safe, and making it thread safe will be a bit annoying

[creamsoup]

those documentation fixes will be nice but still LGTM

[ejona86]

I think that change can be left to a future PR though.

We should not encourage users to migrate before the new API is ready. The main problem is we are @Deprecateding the old API. If we left the old API as-is (not deprecated), then I think it would be fine to do it in a future PR.

Something like:

It isn't clear to me which class you are talking about putting that code in. Since it says "stream" I'd assume ClientCallImpl, but stream is not volatile there. Even if it were there is a race between the two variables stream and pendingRequest that can leave requests un-processed. The logic needs to be based on only one variable.

private final AtomicInteger pendingRequest = new AtomicInteger();

@Override
public void request(int numMessages) {
  checkArgument(numMessages >= 0);
  int oldRequests;
  while ((oldRequests = pendingRequest.get()) >= 0) {
   int newRequests = oldRequests + numMessages;
    if (newRequests < 0) { // overflow
      newRequests = Integer.MAX_VALUE;
    }
    if (pendingRequest.compareAndSet(oldRequests, newRequests)) {
      return;
    }
  }
  stream.request(numMessages);
}

private void setStream(ClientStream stream) {
  this.stream = stream;
  stream.request(pendingRequest.getAndSet(-1));
}

That said, I'm not excited about that logic being in ClientCall, since it is really this specific API's problem. It would probably be better in ClientCalls instead.

However, taking a step back, the options I see:

1. Do nothing
2. Make request() functional starting with beforeStart()
3. Make request() functional starting with beforeStart(), but not thread-safe
4. Allow passing the initial request count to disableAutoRequestWithInitial(int request) (we'd still have disableAutoRequest())
5. Add an onStart() callback (setOnStartHandler()? BetterResponseObserver.afterStart()?)
6. Allow the beforeStart() callback to call start()

(4) is my preference. I'm pretty disappointed with the options overall, but think it is the most straight-forward, causes the least problems, and fits in best with the rest of the API. I wanted to like (1), but couldn't since virtually every user of disableAutoRequest() will have this problem. I don't think it will be a problem to have a different API to issue request() within beforeStart().

[DRayX]

@ejona86

We should not encourage users to migrate before the new API is ready.

Fair enough, I'm good with trying to resolve this in the same PR.

It isn't clear to me which class you are talking about putting that code in. Since it says "stream" I'd assume ClientCallImpl, but stream is not volatile there.

Ya, that would be in ClientCallImpl and I was assuming that stream would be made volatile.

Even if it were there is a race between the two variables stream and pendingRequest that can leave requests un-processed. The logic needs to be based on only one variable.

All early requests will be made on the stream. The setStream method sets the pendingRequests to -1 at which point any requests in the CAS loop abort and are made on the stream directly. There is no way for pending requests to be added after they are consumed. The check on stream == null is just an opportunistic check. The only race is that the pending requests may be made after other requests is called between the stream being set and the pending requests being made, but the order of requests doesn't matter.

That said, I'm not excited about that logic being in ClientCall, since it is really this specific API's problem. It would probably be better in ClientCalls instead.

I'm not actually convinced. I feel like putting it in ClientCall is actually nice as it means that the semantics of request on ClientCall and ServerCall are the same (both can be called whenever).

(4) is my preference.

Don't really love option 4 since it adds an extra method that is used only when setting up ClientCalls without flow control. If we wanted to go that route, I think I would prefer for disableAutoRequest to always have an initialRequests parameter, but documentation would still be needed to clarify that request still can't be called before start in a ClientCall. I feel like allowing ClientCall.request before start is the most intuitive solution.

[ejona86]

The setStream method sets the pendingRequests to -1 at which point any requests...

I see now that the if (stream == null) is not required. Yeah, it looks like it will work. And if the stream == null check was removed then stream would not need to be volatile.

I feel like putting it in ClientCall is actually nice as it means that the semantics of request on ClientCall and ServerCall are the same (both can be called whenever).

I really don't want request() to be able to be called whenever. It adds yet another layer of indirection. And changing the ClientCall interface means that interceptors also have to support this early-calling behavior. And it doesn't really help anyone by being directly in ClientCall. But yes, it keeps ClientCall and the async stubs closer in semantics, and that is nice.

Don't really love option 4 since it adds an extra method that is used only when setting up ClientCalls without flow control.

I'm not wild about it either. It just seemed least bad. Instead of adding one method we add two; unfortunate, but meh.

I think I would prefer for disableAutoRequest to always have an initialRequests parameter

I'm not entirely against that. The main problem I had was that it is unclear what the 0 is if you read disableAutoRequest(0). We could have only disableAutoRequestWithInitial(), but it seemed having the zero-arg method made that method more clear/comprehensible. So even if the zero-arg method was mostly unused, it would only exist within ClientCallStreamObserver and so would be relatively "cheap." I was trying to figure out how often request would be delayed until after start (such that the user would need disableAutoRequestWithInitial(0)), and it didn't seem that common but only because you have to go further out of your way to plumb things together (independent of our API).

I feel like allowing ClientCall.request before start is the most intuitive solution.

I completely agree with the sentiment. But it is also the biggest change (in semantics; not code). I feel wrong making that major of a change for this "minor" of an issue, especially since it is this API's own fault. (ClientCallStreamObserver.request is what I would change first, but that causes the skew issues between ClientCall and StreamObserver and I'd still be bothered by the thread-safety aspect.)

It doesn't seem like the KISS solution; there are a lot of lurking details. That's why I'm favoring the uglier API, because it really is KISS. Easy for vast majority of users. Easy to implement. Easy to comprehend how the APIs interact. Trying to make this part of the API pretty and orthogonal seems to introduce too much cost for the gain. The root problem is that there is no start() method and beforeStart() only approximated it, and fixing that is a whole nother can of worms.

[ejona86]

(FWIW, I thought a long time about the list of options. I really wanted a way to "fix" request such that I could be happy with it. I made up my mind in determining my "preference" multiple times and then hated it still so went back to the drawing board. I'm serious when I say I wanted request() to work. And I do have concerns about disableAutoRequestWithInitial(), but they are mainly concerned with unknown code organization where a user needs a thread-safe request() in beforeStart(). But the choice of disableAutoRequestWithInitial is also settling much nicer for me.)

[ejona86]

The wider team discussed this yesterday and there was a consensus of going with option (4) disableAutoRequestWithInitial(int). On of the views why this was preferred was because it is most similar to the handling of disableAutoInboundFlowControl().

There was not a consensus of whether we should also have disableAutoRequest(). Since "we can always add it later," let's remove it for now.

Does that sound fair?

[DRayX]

Sounds reasonable to me. My one question is what the behavior should be if disableAutoRequestWithInitial is called more than once? My gut feeling is that it should be an illegal state exception, or it should overwrite the initial request count.

[ejona86]

IllegalStateException sounds great.

[DRayX]

As I was implementing, just overwriting the value was simpler, but I'm cool with switching to IllegalStateException if that would be preferred.

[dapengzhang0]

@DRayX Tests are broken, would you be able to fix that?

[ejona86]

Searching for the bug causing test failures I realized that server-side should potentially use disableAutoRequest() and not use disableAutoRequestWithInitial(), since it can safely call request(). The problem with beforeStart() was only client-side, and we didn't discussion server-side's impact when discussing changing to "withinitial". The "withInitial" method "works" on server-side though. But diverging the client/server APIs would mean moving the API into the separate client and server APIs (which is generally fine with me, but more work). I've added an item on our API review agenda to discuss this.

I'd really like this merged. I think it may be best to avoid adding @Deprecated to the old API, and maybe note this new API is still in-progress in the javadoc, and then merge it mostly-as-is. Then we do any other work as follow-up.

[DRayX]

Thanks for finding the bug, sorry I hadn't looked at it yet, I got sidetracked on another project.
I'm cool with removing the @Deprecated annotations and adding @ExperimentalApi to the new one.

[ejona86]

@DRayX, thank you! Sorry that was quite a process!

[ejona86]

@DRayX, we discussion in the api review meeting today. I posted results to #1788 (comment) . But basically we're going to have disableAutoRequest on server-side and disableAutoRequestWithInitial on client-side.

I was planning on making a PR with the changes.