[ISSUE #8714] Prevent receive message NPE when client settings are missing by Aias00 · Pull Request #10585 · apache/rocketmq

Aias00 · 2026-07-03T03:48:14Z

Which Issue(s) This PR Fixes

Brief Description

ReceiveMessageActivity currently assumes GrpcClientSettingsManager#getClientSettings always returns a non-null Settings instance. When receive-message is called before client settings are cached, the proxy dereferences settings.getClientType() / settings.getSubscription() and returns an internal NPE instead of a structured receive response.

This PR treats missing client settings the same as the existing default-settings path used by tests, allowing receive validation and pop handling to continue without throwing a NullPointerException.

How Did You Test This Change?

mvn -pl proxy -Dtest=ReceiveMessageActivityTest test

Result: BUILD SUCCESS, Tests run: 10, Failures: 0, Errors: 0, Skipped: 0

Copilot

Pull request overview

This PR addresses issue #8714 by preventing a NullPointerException in the gRPC proxy’s ReceiveMessageActivity when client Settings are not yet cached, and adds a regression test covering the missing-settings scenario.

Changes:

Add a null-guard in ReceiveMessageActivity#receiveMessage to handle GrpcClientSettingsManager#getClientSettings returning null.
Add a unit test verifying receive-message returns a structured response when client settings are missing.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
proxy/src/main/java/org/apache/rocketmq/proxy/grpc/v2/consumer/ReceiveMessageActivity.java	Adds a fallback path when client settings are missing to avoid NPE during receive-message handling.
proxy/src/test/java/org/apache/rocketmq/proxy/grpc/v2/consumer/ReceiveMessageActivityTest.java	Adds a regression test ensuring missing client settings don’t cause an internal error response.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

            Settings settings = this.grpcClientSettingsManager.getClientSettings(ctx);
+            if (settings == null) {
+                settings = Settings.getDefaultInstance();
+            }
            final boolean isLite = ClientType.LITE_PUSH_CONSUMER.equals(settings.getClientType());


codecov-commenter · 2026-07-03T04:18:22Z

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 48.17%. Comparing base (8242c1e) to head (8f6a9c9).
⚠️ Report is 3 commits behind head on develop.

Additional details and impacted files

@@              Coverage Diff              @@
##             develop   #10585      +/-   ##
=============================================
- Coverage      48.26%   48.17%   -0.10%     
+ Complexity     13433    13413      -20     
=============================================
  Files           1378     1378              
  Lines         100817   100823       +6     
  Branches       13040    13042       +2     
=============================================
- Hits           48660    48569      -91     
- Misses         46211    46293      +82     
- Partials        5946     5961      +15

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

RockteMQ-AI

Review by github-manager-bot

Summary

Adds a null-guard for getClientSettings() in ReceiveMessageActivity.receiveMessage() to prevent NPE when client settings have not yet been cached, returning a structured receive response instead of an internal error.

Findings

[Info] proxy/.../ReceiveMessageActivity.java:68 — Using Settings.getDefaultInstance() as the fallback is reasonable and consistent with how other activities handle missing settings. The downstream ClientType.LITE_PUSH_CONSUMER.equals(...) will safely return false for the default instance, and getSubscription() on a default Settings returns an empty subscription — so the flow degrades gracefully.
[Info] proxy/.../ReceiveMessageActivityTest.java — Test coverage is solid: mocks getClientSettings to return null and verifies the response code. Good practice.

Suggestions

Consider whether sendMessageActivity or other gRPC v2 activities have the same gap. A quick audit of all getClientSettings(ctx) call sites without null checks would be a useful follow-up.

Automated review by github-manager-bot

ReceiveMessageActivity can be invoked before cached gRPC client settings are available. When settings are missing, fall back to the real default consumer settings (maxAttempts = retryMaxTimes + 1) instead of Settings.getDefaultInstance(), whose backoffPolicy.maxAttempts is 0. With maxAttempts == 0, PopMessageResultFilterImpl routes fresh messages (reconsumeTimes == 0) straight to the DLQ via reconsumeTimes >= maxAttempts. The real default avoids both the NullPointerException and the DLQ regression. Constraint: Related to apache#8714 Confidence: high Scope-risk: narrow Directive: Keep missing client settings on the default consumer path unless the protocol starts requiring an explicit settings handshake before receive. Tested: mvn -pl proxy -Dtest=ReceiveMessageActivityTest,PopMessageResultFilterImplTest,GrpcClientSettingsManagerTest test Not-tested: Full repository test suite

Aias00 · 2026-07-11T15:36:22Z

Re: the suggestion to audit other getClientSettings(ctx) call sites — done. The three other call sites are safe:

ClientActivity#heartbeat (line 102): already has an explicit if (clientSettings == null) guard returning UNRECOGNIZED_CLIENT_TYPE.
ClientActivity#processClientSettings (line 406): calls updateClientSettings(...) immediately before the read, so settings are guaranteed cached.
GrpcClientChannel#getChannelExtendAttribute (line 84): uses getRawClientSettings with an explicit if (settings == null) return null; guard.

The producer activities (SendMessageActivity, RecallMessageActivity, ForwardMessageToDLQActivity) don't read client settings at all — they only thread the manager through the constructor.

So ReceiveMessageActivity was the only site that dereferenced the settings without a null check; no further changes needed.

RockteMQ-AI

Review by github-manager-bot (Re-review)

Summary

New commit e5f7c3a improves the null-settings fallback by replacing Settings.getDefaultInstance() with a proper getDefaultConsumerSettings() that carries a real maxAttempts value, preventing fresh messages from being routed to the DLQ when client settings are not yet cached.

Changes Since Last Review

GrpcClientSettingsManager.java — Added getDefaultConsumerSettings() that delegates to createDefaultConsumerSettingsBuilder().build(), ensuring maxAttempts = retryMaxTimes + 1 instead of the protobuf empty default of 0.
ReceiveMessageActivity.java:68 — Fallback now calls getDefaultConsumerSettings() instead of Settings.getDefaultInstance().
PopMessageResultFilterImplTest.java — New test class with 3 test cases covering the DLQ regression path (fresh message, retry exhaustion, null settings).
GrpcClientSettingsManagerTest.java — New test verifying maxAttempts > 0 on the default consumer settings.
ReceiveMessageActivityTest.java — New test verifying the fallback path does not DLQ fresh messages.

Findings

[Positive] The fix correctly addresses the subtle DLQ risk: with protobuf's empty default, backoffPolicy.maxAttempts == 0 causes PopMessageResultFilterImpl to treat reconsumeTimes == 0 as exhausted. The new default prevents this.
[Positive] Test coverage is thorough — covers both the settings layer and the filter layer, with clear test names explaining the regression scenario.
[Info] proxy/.../ReceiveMessageActivity.java:68 — The fallback getDefaultConsumerSettings() creates a new Settings object on every call when settings are null. If this path is hot, consider caching the default. However, this is a minor concern since the null-settings case should be rare (only before first client registration).

Verdict

The improvement is well-executed. The previous review's suggestion about auditing other getClientSettings() call sites remains a useful follow-up.

Automated review by github-manager-bot

Copilot AI review requested due to automatic review settings July 3, 2026 03:48

Aias00 force-pushed the fix/settings-null-npe branch from 203e34b to 272e93f Compare July 3, 2026 03:48

Copilot started reviewing on behalf of Aias00 July 3, 2026 03:48 View session

Copilot AI reviewed Jul 3, 2026

View reviewed changes

RockteMQ-AI reviewed Jul 11, 2026

View reviewed changes

Aias00 force-pushed the fix/settings-null-npe branch from 272e93f to 8f6a9c9 Compare July 11, 2026 15:32

RockteMQ-AI reviewed Jul 11, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[ISSUE #8714] Prevent receive message NPE when client settings are missing#10585

[ISSUE #8714] Prevent receive message NPE when client settings are missing#10585
Aias00 wants to merge 1 commit into
apache:developfrom
Aias00:fix/settings-null-npe

Aias00 commented Jul 3, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

codecov-commenter commented Jul 3, 2026 •

edited

Loading

Uh oh!

RockteMQ-AI left a comment

Uh oh!

Aias00 commented Jul 11, 2026

Uh oh!

RockteMQ-AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

Aias00 commented Jul 3, 2026

Which Issue(s) This PR Fixes

Brief Description

How Did You Test This Change?

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

codecov-commenter commented Jul 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Review by github-manager-bot

Summary

Findings

Suggestions

Uh oh!

Aias00 commented Jul 11, 2026

Uh oh!

RockteMQ-AI left a comment

Choose a reason for hiding this comment

Review by github-manager-bot (Re-review)

Summary

Changes Since Last Review

Findings

Verdict

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-commenter commented Jul 3, 2026 •

edited

Loading