Fix misleading NPE when request parameter is not Serializable#16299
Fix misleading NPE when request parameter is not Serializable#16299chensishang wants to merge 4 commits into
Conversation
When a Dubbo service is called with non-serializable parameters, the invocation deserialization fails silently and `path` becomes null. This null path is then passed to GroupServiceKeyCache which uses a ConcurrentHashMap that does not accept null keys, resulting in a confusing NullPointerException. Added a null check for `path` in DubboProtocol.getInvoker() to throw a clear RemotingException with a meaningful message pointing users to the actual cause (non-serializable parameters). Fixes apache#16293 Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## 3.3 #16299 +/- ##
============================================
- Coverage 60.80% 60.78% -0.03%
- Complexity 11766 11768 +2
============================================
Files 1953 1953
Lines 89188 89211 +23
Branches 13454 13459 +5
============================================
- Hits 54235 54229 -6
- Misses 29391 29409 +18
- Partials 5562 5573 +11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hi reviewers, I noticed the Codecov check failed with "0% coverage". After checking, this seems to be due to an incomplete report upload ("HEAD比BASE少7次上传") rather than a problem with the code change itself. I have already added a unit test (testGetInvokerThrowsOnNullPath) which covers the new null-check logic, and it passes locally. The Codecov result appears to be a false negative caused by the CI infrastructure. Could you please take another look? Thanks! |
zrlw
added
type/enhancement
|
Thanks for the review! |
|
Hi @zrlw, thanks for the review! The PR contains two commits: the actual fix ( The maintainer can squash on merge if needed. Sorry for the noise! |
| int port = channel.getLocalAddress().getPort(); | ||
| String path = (String) inv.getObjectAttachmentWithoutConvert(PATH_KEY); | ||
| if (path == null) { | ||
| throw new RemotingException( |
There was a problem hiding this comment.
Is non-serializable parameter really the only scenario that can make path null here? In the normal Dubbo request format, the service path is encoded/decoded before arguments, and DubboInvoker also sets PATH_KEY before sending the request. So a null path seems more generally like missing/malformed invocation metadata, custom codec/invocation behavior, or decode/protocol incompatibility. Could we make this error message less specific than “Please ensure all parameter types implement Serializable” to avoid another misleading diagnosis?
There was a problem hiding this comment.
Thanks for the careful review — you're right, and I appreciate you pointing this out. A null path indicates missing or corrupted invocation metadata in general, and my original message attributing ▎ it solely to non-serializable parameters could indeed cause another misleading diagnosis
I've updated the message to describe the problem generically and only mention deserialization failure as one of several possible causes (along with protocol incompatibility and custom
codec/invocation implementations). Please let me know if the wording could be further improved — happy to adjust.
|
Sorry for the extra round — the review above pointed out that my error message was too specific, so I've updated the wording, which dismissed your earlier approval. Could you take another |
4 participants
When a Dubbo service is called with non-serializable parameters, the invocation deserialization fails silently and
pathbecomes null. This null path is then passed to GroupServiceKeyCache which uses a ConcurrentHashMap that does not accept null keys, resulting in a confusing NullPointerException.Added a null check for
pathin DubboProtocol.getInvoker() to throw a clear RemotingException with a meaningful message pointing users to the actual cause (non-serializable parameters).Fixes #16293
What is the purpose of the change?
Checklist