Skip to content

Commit 15d64a4

Browse files
authored
core: Reject retryThrottling without maxTokens or tokenRatio clearly (grpc#13131)
## Problem `ServiceConfigUtil.getThrottlePolicy()` reads the two required fields of `retryThrottling` with `JsonUtil.getNumberAsDouble(...).floatValue()`. When either field is missing, `getNumberAsDouble` returns null and the call fails with an NPE: Cannot invoke "java.lang.Double.floatValue()" because the return value of "io.grpc.internal.JsonUtil.getNumberAsDouble(java.util.Map, String)" is null `ScParser` catches it and the channel reports `UNKNOWN: failed to parse service config`, but the cause doesn't say which field is missing. This was noted in the code as `TODO(dapengzhang0): check if this is null.` ## Fix Check both fields with `checkNotNull(..., "<field> cannot be empty")`, the same message used for the required `retryPolicy` and `hedgingPolicy` fields in `ManagedChannelServiceConfig`. The exception type is still the NPE that `ScParser` already handles, so the only change is a clearer message. ## Testing Added `getThrottlePolicy_failOnMissingMaxTokens` and `getThrottlePolicy_failOnMissingTokenRatio` to `ServiceConfigUtilTest`. Both failed before the fix with the NPE above.
1 parent 14a27ad commit 15d64a4

2 files changed

Lines changed: 28 additions & 3 deletions

File tree

‎core/src/main/java/io/grpc/internal/ServiceConfigUtil.java‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -112,9 +112,12 @@ static Throttle getThrottlePolicy(@Nullable Map<String, ?> serviceConfig) {
112112
return null;
113113
}
114114

115-
// TODO(dapengzhang0): check if this is null.
116-
float maxTokens = JsonUtil.getNumberAsDouble(throttling, "maxTokens").floatValue();
117-
float tokenRatio = JsonUtil.getNumberAsDouble(throttling, "tokenRatio").floatValue();
115+
float maxTokens = checkNotNull(
116+
JsonUtil.getNumberAsDouble(throttling, "maxTokens"),
117+
"maxTokens cannot be empty").floatValue();
118+
float tokenRatio = checkNotNull(
119+
JsonUtil.getNumberAsDouble(throttling, "tokenRatio"),
120+
"tokenRatio cannot be empty").floatValue();
118121
checkState(maxTokens > 0f, "maxToken should be greater than zero");
119122
checkState(tokenRatio > 0f, "tokenRatio should be greater than zero");
120123
return new Throttle(maxTokens, tokenRatio);

‎core/src/test/java/io/grpc/internal/ServiceConfigUtilTest.java‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -114,6 +114,28 @@ public void unwrapLoadBalancingConfigList_failOnMalformedConfig() throws Excepti
114114
}
115115
}
116116

117+
@Test
118+
public void getThrottlePolicy_failOnMissingMaxTokens() throws Exception {
119+
String serviceConfig = "{\"retryThrottling\" : {\"tokenRatio\" : 0.1}}";
120+
try {
121+
ServiceConfigUtil.getThrottlePolicy(checkObject(JsonParser.parse(serviceConfig)));
122+
fail("Should throw");
123+
} catch (Exception e) {
124+
assertThat(e).hasMessageThat().contains("maxTokens cannot be empty");
125+
}
126+
}
127+
128+
@Test
129+
public void getThrottlePolicy_failOnMissingTokenRatio() throws Exception {
130+
String serviceConfig = "{\"retryThrottling\" : {\"maxTokens\" : 10}}";
131+
try {
132+
ServiceConfigUtil.getThrottlePolicy(checkObject(JsonParser.parse(serviceConfig)));
133+
fail("Should throw");
134+
} catch (Exception e) {
135+
assertThat(e).hasMessageThat().contains("tokenRatio cannot be empty");
136+
}
137+
}
138+
117139
@SuppressWarnings("unchecked")
118140
private static List<Map<String, ?>> checkObjectList(Object o) {
119141
return (List<Map<String, ?>>) o;

0 commit comments

Comments
 (0)