core: Added changes to DelayedStream.setStream() should cancel the provided stream if not using it #11969
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ovided stream if not using it
vinodhabib
reviewed
Mar 19, 2025
| @@ -271,14 +272,41 @@ public void uncaughtException(Thread t, Throwable e) { | |||
| verifyNoMoreInteractions(mockRealStream); | |||
| } | |||
|
|
|||
| @Test | |||
| public void newStreamThenShutDownNow() { | |||
There was a problem hiding this comment.
append test with UT method name -> testNewStreamThenShutDownNow()
vinodhabib
reviewed
Mar 19, 2025
| ClientStream stream = delayedTransport.newStream( | ||
| method, new Metadata(), CallOptions.DEFAULT, tracers); | ||
| stream.start(streamListener); | ||
| assertEquals(1,delayedTransport.getPendingStreamsCount()); |
There was a problem hiding this comment.
add an empty line above asserts statements in all applicable places.
vinodhabib
reviewed
Mar 19, 2025
…ovided stream if not using it
vinodhabib
reviewed
Mar 21, 2025
| return null; | ||
| } | ||
|
|
There was a problem hiding this comment.
remove an empty line here
AgraVator
reviewed
Apr 2, 2025
| synchronized (this) { | ||
| // If realStream != null, then either setStream() or cancel() has been called. |
There was a problem hiding this comment.
No, added back the removed comment, it was accidentally missed while updating the code, but the logic still holds. if realStream() != null, it means setStream() was called, and if listener !=null, the old stream may have been cancelled.
AgraVator
reviewed
Apr 2, 2025
| ClientStream stream = delayedTransport.newStream( | ||
| method, new Metadata(), CallOptions.DEFAULT, tracers); | ||
| stream.start(streamListener); | ||
| assertEquals(1,delayedTransport.getPendingStreamsCount()); |
There was a problem hiding this comment.
Please add a space after the comma...
AgraVator
reviewed
Apr 2, 2025
| verify(streamListener).closed( | ||
| statusCaptor.capture(), any(RpcProgress.class), any(Metadata.class)); | ||
|
|
||
| assertEquals(0,delayedTransport.getPendingStreamsCount()); |
There was a problem hiding this comment.
Please add a space after the comma...
This allows Filters to access the xds configuration for their own processing. From gRFC A83: > This data is available via the XdsConfig attribute introduced in A74. > If the xDS ConfigSelector is not already passing that attribute to the > filters, it will need to be changed to do so.
It is much harder to debug refcounting problems when we ignore impossible situations. So make such impossible cases complain loudly so the bug is obvious.
This is to support gRFC A83 xDS GCP Authentication Filter: > Otherwise, the filter will look in the CDS resource's metadata for a > key corresponding to the filter's instance name.
panic() calls a good amount of code, so it could get another exception. The SynchronizationContext is running on an arbitrary thread and we don't want to propagate this secondary exception up its stack (to be handled by its UncaughtExceptionHandler); it we wanted that we'd propagate the original exception. This second exception will only be seen in the logs; the first exception was logged and will be used to fail RPCs. Also related to http://yaqs/8493785598685872128 and b692b9d
…already have their connections cancelled (grpc#11934) Some clients watching health status can cancel their watch and `HealthService` when trying to notify these watchers were getting CANCELLED exception because there was no cancellation handler set on the `StreamObserver`. This change sets the cancellation handler that removes the watcher from the set of watcher clients to be notified of the health status.
Previously it would wait for the new LB to enter READY. However, that prevents there being an upper-bound on how long the old policy will continue to be used. The point of graceful switch is to avoid RPCs seeing increased latency when we swap config. We don't want it to prevent the system from becoming eventually consistent.
I think at some point there were more usages in the tests. But now it is pretty easy. PriorityLb.ChildLbState.picker is initialized to FixedResultPicker(NoResult). So now that GracefulSwitchLb is using the same picker, equals() is able to de-dup an update.
…CING_CONFIG (grpc#11982) ATTR_LOAD_BALANCING_CONFIG was deleted in bf7a42d.
"EXP" stood for experimental and all documentation that referenced it made it clear it was experimental. It's been some years since we started logging a message when it was used to say it will be deleted. There's no time like the present to delete it.
…ovided stream if not using it
…ovided stream if not using it
This reverts commit 67b2f3a.
…ed_1537 # Conflicts: # core/src/test/java/io/grpc/internal/DelayedClientTransportTest.java
|
@AgraVator Gentle reminder on review.. |
# for free
to join this conversation on GitHub.
Already have an account?
# to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
core: Added changes to DelayedStream.setStream() should cancel the provided stream if not using it
Fixes: #1537