core: exclude client and hedging cancellations from callcounters for outlier-detection

[AgraVator]

fixes #12834

[kannanjgithub]

This method is for transport callback for stream closure and it is too late at this point to make a distinction whether the cancellation is app/retriable stream initiated or from the remote endpoint. This should instead be done in method cancel that sets this.cancelled = true; and that call only happens for app/retriable stream initiated cancellation, and not for remote server initiated cancellations, and happens before the transport stream is closed.

[kannanjgithub]

In addition, we would need the guard in AbstractClientStream.cancel against late cancellations:

     @Override
     public final void cancel(Status reason) {
       Preconditions.checkArgument(!reason.isOk(), "Should not cancel with OK status");
       
       if (cancelled || transportState().listenerClosed) {   //  <----- GUARD 
         return;
       }
       
      cancelled = true;
      statsTraceCtx.clientCancelled(reason);
      abstractClientStreamSink().cancel(reason);
    }

[AgraVator]

Placing statsTraceCtx.clientCancelled(reason) inside cancel() on the calling thread (app thread or deadline timer thread) could result in a race condition. Had it like that in the initial commits but then eric suggested to use transportReadyStatus() instead.

[kannanjgithub]

We need to add unit tests for these changes in BinderClientTransportTest.java. testCancelStream_notifiesTracer in it already tests client initiated cancellation, we can enhance the same.

[kannanjgithub]

Using stopDelivery == true as the only indicator of a client-initiated cancellation is flawed because its meaning is context-dependent:

1. Client Cancels (App / Hedging): Passes stopDelivery = true.
2. Client Timeouts (DEADLINE_EXCEEDED): Passes stopDelivery = true.
3. Transport Forceful Shutdown (UNAVAILABLE): Passes stopDelivery = true.
4. Abrupt Network Drop (UNAVAILABLE): Passes stopDelivery = false.
5. Server Sends Reset (CANCEL or INTERNAL): Passes stopDelivery = false.

(3) is a problem. In Netty transport for example, NettyClientHandler.forcefulClose explicitly sets stopDelivery = true (passing Status.UNAVAILABLE or similar).

Change this to:

         // We must ensure the status code actually reflects a client-initiated action!
         if (stopDelivery && (status.getCode() == Status.Code.CANCELLED || 
                             status.getCode() == Status.Code.DEADLINE_EXCEEDED)) {
          statsTraceCtx.clientCancelled(status);
          }

[ejona86]

No, we definitely shouldn't be doing any logic here based on the status code.

What is wrong with "Transport Forceful Shutdown'? That is defined as cancelling all streams, so it should behave the same as call.cancel().

I can believe we may need to change some cases to stop using stopDelivery=true; I bet there are/were some cases where it previously didn't matter what the value was. I think we can rename stopDelivery to cancelled, as that's the only time we should be discarding data that we've received.

[kannanjgithub]

I wanted to count transport forceful shutdown in response to network drop as failure because it is not client app initiated although it might be client network stack initiated. Envoy does count connection drops as failures (LocalOriginConnectFailed) without any HTTP status code received from the remote peer.

[ejona86]

Why are we doing this? I'm worried this is misleading, as this doesn't prevent cancel() from happening twice because it is racy. And do we actually need to prevent this from happening twice, since it is within the transport thread that we'll check listenerClosed.

Also, this is not thread-safe, as listenerClosed is being accessed from multiple threads concurrently.

[kannanjgithub]

This was added with my previous suggestion where we were calling statsTraceCtx.clientCancelled(status); from this method, and there was a need to check if the transport had already closed. With the new approach of stopDelivery, this check is no longer required and we don't need the method TransportState.isListenerClosed.
Yes, the thread-unsafe read was overlooked.

[ejona86]

No, we definitely shouldn't be doing any logic here based on the status code.

What is wrong with "Transport Forceful Shutdown'? That is defined as cancelling all streams, so it should behave the same as call.cancel().

I can believe we may need to change some cases to stop using stopDelivery=true; I bet there are/were some cases where it previously didn't matter what the value was. I think we can rename stopDelivery to cancelled, as that's the only time we should be discarding data that we've received.

[ejona86]

I think we could call statsTraceCtx.clientCancelled() from transportReportStatus and just ignore whether listenerClosed and have the tracer figure out whether the cancellation actually mattered. Or we could check listenerClosed within transportReportStatus(). Calling clientCancelled within transportReportStatus will deliver the cancellation notification sooner, which seems beneficial. Close() has to be delayed until all resources are cleaned up because after close we can't use them any more, but cancellation notification wouldn't need to be. But overall, I think some variations here wouldn't matter much for years (that is to say, the current code is fair).

[ejona86]

The reason we access variables in the transport state from methods is for the method to provide synchronization guarantees within the scope of this class (where it is easy to check). This is obviously not providing any synchronization and thus is broken.