Skip to content

fix: Fix race condition in GrpcDirectStreamController#1537

Merged
igorbernstein2 merged 2 commits intogoogleapis:mainfrom
mutianf:race
Mar 23, 2023
Merged

fix: Fix race condition in GrpcDirectStreamController#1537
igorbernstein2 merged 2 commits intogoogleapis:mainfrom
mutianf:race

Conversation

@mutianf
Copy link
Contributor

@mutianf mutianf commented Mar 22, 2023 •
edited
Loading

Fix the race condition in GrpcDirectStreamController when controller.request() is called when there's a retry attempt. This could cause IllegalStateException:

java.lang.IllegalStateException: Not started

	at com.google.common.base.Preconditions.checkState(Preconditions.java:502)
	at io.grpc.internal.ClientCallImpl.request(ClientCallImpl.java:451)
	at com.google.api.gax.grpc.GrpcDirectStreamController.request(GrpcDirectStreamController.java:95)
	at com.google.api.gax.grpc.ExceptionResponseObserver$1.request(ExceptionResponseObserver.java:67)
	at com.google.api.gax.rpc.ServerStreamingAttemptCallable.onRequest(ServerStreamingAttemptCallable.java:345)
	at com.google.api.gax.rpc.ServerStreamingAttemptCallable.access$200(ServerStreamingAttemptCallable.java:97)
	at com.google.api.gax.rpc.ServerStreamingAttemptCallable$1.request(ServerStreamingAttemptCallable.java:168)
	at com.google.api.gax.tracing.TracedResponseObserver$1.request(TracedResponseObserver.java:83)
	at com.google.api.gax.rpc.ServerStreamingAttemptCallable.onRequest(ServerStreamingAttemptCallable.java:345)
	at com.google.api.gax.rpc.ServerStreamingAttemptCallable.access$200(ServerStreamingAttemptCallable.java:97)
	at com.google.api.gax.rpc.ServerStreamingAttemptCallable$1.request(ServerStreamingAttemptCallable.java:168)

This get triggered when GrpcDirectStreamController is created from a retry thread and controller.request() get called from another thread:

this.hasStarted = true;

// controller.request() called from the another thread

clientCall.start(new ResponseObserverAdapter(), new Metadata());

@mutianf mutianf requested a review from a team March 22, 2023 18:53
@product-auto-label product-auto-label bot added the size: m Pull request size is medium. label Mar 22, 2023
@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 22, 2023

[gapic-generator-java-root] Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

100.0% 100.0% Coverage
0.0% 0.0% Duplication

blakeli0
blakeli0 reviewed Mar 22, 2023
View reviewed changes
gax-java/gax-grpc/src/main/java/com/google/api/gax/grpc/GrpcDirectStreamController.java

clientCall.start(new ResponseObserverAdapter(), new Metadata());

this.hasStarted = true;
Copy link
Contributor

@blakeli0 blakeli0 Mar 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why this line is moved from before clientCall.start to after it? I don't know the exact use case of startCommon, but it could be an issue if clientCall.start is taking some time to complete and startCommon is being called another time.

Copy link
Contributor Author

@mutianf mutianf Mar 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hasStarted is meant to be a barrier before we could interact with a stream, grpc has the invariant that you can't call request before a stream is started. Before the barrier is opened, we simply increment the request count. The original assumption is that the only thing that could call request() this is in onStart() and onResponse(). However this is not the case when retry is enabled where controller could be created from a different thread.

I don't think clientCall.start() taking a long time will be a problem here. GrpcDirectStreamController#start() is only called from here and here for bidi, which means that every callable.call() will create a new controller instance, so I don't think there's a race condition here.

Copy link
Contributor

@blakeli0 blakeli0 Mar 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, it makes sense.

@blakeli0 blakeli0 added the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 22, 2023
@gcf-owl-bot gcf-owl-bot bot removed the owlbot:run Add this label to trigger the Owlbot post processor. label Mar 22, 2023
@igorbernstein2 igorbernstein2 merged commit 17d133b into googleapis:main Mar 23, 2023
@release-please release-please bot mentioned this pull request Mar 23, 2023
Closed
@mutianf mutianf deleted the race branch March 23, 2023 15:50
@release-please release-please bot mentioned this pull request Mar 28, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@blakeli0 blakeli0 blakeli0 approved these changes

Assignees

No one assigned

Labels

size: m Pull request size is medium.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mutianf @blakeli0 @igorbernstein2