Skip to content

Commit

Permalink
grpc-netty: fix flaky SingleRequestOrResponseApiTest (#3064)
Browse files Browse the repository at this point in the history
Motivation:

We have a flaky test. It turns out that it should always fail, but we
get lucky and short circuit a `Publisher.cancel()` call the vast
majority of the time. However, fixing that is going to be relatively
hard/ugly.

Modifications:

Because we're guarding against bad gRPC implementations, not app usage,
and the server behavior is correct (it doesn't process the message in
any case), the best thing to do is relax the test requirements and
accept the CANCEL result for the cases where it wins the race. This
result is slightly less debuggable from the client perspective, but the
behavior is acceptable.

Result:

One less flaky test.
  • Loading branch information
bryce-anderson authored Sep 26, 2024
1 parent da43c56 commit c063483
Showing 1 changed file with 11 additions and 2 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@
import static io.servicetalk.concurrent.api.Publisher.from;
import static io.servicetalk.concurrent.api.Publisher.fromIterable;
import static io.servicetalk.concurrent.api.Single.defer;
import static io.servicetalk.grpc.api.GrpcStatusCode.CANCELLED;
import static io.servicetalk.grpc.api.GrpcStatusCode.INVALID_ARGUMENT;
import static io.servicetalk.transport.netty.internal.AddressUtils.localAddress;
import static io.servicetalk.transport.netty.internal.AddressUtils.serverHostAndPort;
Expand All @@ -64,6 +65,7 @@
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.oneOf;
import static org.junit.jupiter.api.Assertions.assertThrows;
import static org.junit.jupiter.api.Assumptions.assumeFalse;

Expand Down Expand Up @@ -137,8 +139,15 @@ private void serverResponseStreamingRouteFailsWithInvalidArgument(Iterable<TestR
try (BlockingTesterClient client = newBlockingClient()) {
GrpcStatusException e = assertThrows(GrpcStatusException.class,
() -> client.testBiDiStream(requestItems).forEach(response -> { /* noop */ }));
assertThat(e.status().code(), is(INVALID_ARGUMENT));
assertThat(e.status().description(), equalTo(expectedMsg));
// Note that the ideal solution would be to always receive the `INVALID_ARGUMENT` result, but our current
// synchronous implementation makes that challenging to do and the refactor necessary to make it work is not
// currently worth it. While less optimal, the CANCELLED result is also an _acceptable_ result because what
// we really want is for the server to not process the flow.
// See the notes in the GH issue #2399 for more details.
if (e.status().code() != CANCELLED) {
assertThat(e.status().code(), is(oneOf(INVALID_ARGUMENT)));
assertThat(e.status().description(), equalTo(expectedMsg));
}
}
}

Expand Down

0 comments on commit c063483

Please sign in to comment.