xds: Update logic so that an error being reported when stream is closed gets propagated to subscribers

[larry-safran]

Eliminated waitForReady in XdsClient's stub. (Addresses the needs of TD Client Migration team)

The AbstractAdsStream.handleRpcStreamClosed method is used to handle both rpc error and rpc completed. Previously, if AbstractAdsStream.closed == true, then all the logic in handleRpcStreamClosed was skipped. Now it is only skipped if shutdown == true.

Additionally, adds some cleanup.

[YifeiZhuang]

I have question about the reason behind regarding the handleRpcStreamClosed part.
Do we see use cases that AdsStreams close improperly? Did we see handleRpcStreamClosed() called multiple times? Why we need the fix?

[YifeiZhuang]

are these lines necessary after Assert.fail()? Also they seem incomplete about error verification.

Also it looks the test are not strictly testing the "narrowing down close condition" change at all.

It is a bit redundant with test case streamClosedAndRetryWithBackoff, if you wanted to test the close/retry.

The IllegalArgument may not be related to stream creation failure but about the xdstp thing?

[larry-safran]

I put this in to verify that a malformed URL got processed in the expected way which was different than how a nonexistent host was handled. It had nothing to do with close or retry. Handling unknown URLs is what caused the user's problem and I noticed that both unknown and invalid url cases were missing.

Cleaned up the lines after the Assert.fail.

[larry-safran]

In regards to handleRpcStreamClosed, it is used by both handleRpcCompleted() and handleRpcError(). In the case of the unknown URL, the AdsStream() hasn't ever been opened, so closed is true, but we want to propagate the error up to the subscribed observers. This change stopped the bypassing of xdsResponseHandler.handleStreamClosed(error).

[YifeiZhuang]

In regards to handleRpcStreamClosed, it is used by both handleRpcCompleted() and handleRpcError(). In the case of the unknown URL, the AdsStream() hasn't ever been opened, so closed is true, but we want to propagate the error up to the subscribed observers. This change stopped the bypassing of xdsResponseHandler.handleStreamClosed(error).

1. if adstream() has not been opened, the closed by default is false, isn't it?
2. if there is no stream, how does it receive onError or onComplete?

I guess creating resourceSubscriber should never fail.
I probably missed something, can i take a look at stacktrace or debug message?

[larry-safran]

I believe that you are correct that it wouldn't have mattered whether it was closed or shutdown that was checked. I definitely see handleRpcStreamClosed called after shutdown, so there needs to be some sort of check so that the rpcRetryTimer doesn't get triggered and shutdown is more descriptive of why that check is in place.

[YifeiZhuang]

There are side effects of shutdown check instead of close check: already closed stream may get retry if handleRpcStreamClosed called twice.

Also, expected behaviour of creating ResourceSubscribe is that is does not throw. If getServerInfo fails, the xdsChannel is null. Then there will be no AdsStream and the watcher will still be notified with error.