oxsean commented on PR #16325: URL: https://github.com/apache/dubbo/pull/16325#issuecomment-4809138602
Reviewed against the current `3.3` base. The core disambiguation logic is sound — using exact wrapper arg-type matching and only accepting a generated unary/server-stream pair when both parameter and response types match correctly separates a real generated twin from a same-named user overload. A few points worth a look: **1. [correctness] The reflection path can now return `null` for a 2-method overload that isn't a verified generated pair, and the caller NPEs on it.** `DescriptorUtils.java:104` → `findGeneratedUnaryMethodDescriptor` (`:184-213`) The non-gRPC listeners (`DefaultHttp11ServerTransportListener`, `GenericHttp2ServerTransportListener`) resolve via `buildRpcInvocation → findMethodDescriptor → findReflectionMethodDescriptor`, which has no `rawMessage` to disambiguate. Previously, for exactly two same-named methods where one was `SERVER_STREAM`, the code returned the unary method. Now `findSingleOrGeneratedUnaryMethodDescriptor` returns `null` unless the pair is a verified unary/server-stream twin (matching param classes AND response types). For a genuine overload such as `X foo(A)` (unary) + `void foo(A, StreamObserver<Y>)` (server-stream) with `Y != X`, it now returns `null`. `AbstractServerTransportListener#buildRpcInvocation` does not null-check the result and dereferences it (`methodDescriptor.getMethodName()`), so this becomes an NPE rather than a clean error. Consider throwing `UnimplementedException` for the ambiguous case instead of returning `null`. **2. [behavior change] `parseRequestWrapper` swallows parse failures, which can misroute a corrupt wrapper instead of surfacing the decode error.** `DescriptorUtils.java:162-171` Previously a `TripleRequestWrapper.parseFrom` failure propagated. The new helper catches `IOException | RuntimeException` and returns `null`; with `argTypes == null` resolution then falls through to `findGeneratedUnaryMethodDescriptor`. For a verified unary/server-stream pair, a truncated/corrupt first frame now silently resolves to the unary method and is deserialized against it, turning a clear decode error into a misrouted call. Worth confirming this is intended. **3. [cleanup] Duplicated well-known-method dispatch.** `DescriptorUtils.java:92-101` and `:111-120` The identical `isGeneric → genericService()...` / `isEcho → echoService()...` prefix is now in both `findReflectionMethodDescriptor` and `findTripleMethodDescriptor`; extracting a shared helper keeps the two in sync. (Minor, pre-existing: the class Javadoc at line 38 still says "The MetaUtils provides...".) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
