core: Reject retryThrottling without maxTokens or tokenRatio clearly by JoeCqupt · Pull Request #13131 · grpc/grpc-java · GitHub
Skip to content

core: Reject retryThrottling without maxTokens or tokenRatio clearly - #13131

Open
JoeCqupt wants to merge 1 commit into
grpc:masterfrom
JoeCqupt:fix/retry-throttling-null-check
Open

JoeCqupt wants to merge 1 commit into
grpc:masterfrom
JoeCqupt:fix/retry-throttling-null-check

Conversation

@JoeCqupt

Copy link
Copy Markdown
Contributor

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.

Both fields are required, but a missing one caused an NPE from
Double.floatValue() with no hint of which field was absent. Check them
explicitly, matching the "cannot be empty" messages used for the
required retryPolicy and hedgingPolicy fields.
@JoeCqupt

Copy link
Copy Markdown
Contributor Author

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant