Aias00 commented on code in PR #7273:
URL: https://github.com/apache/shenyu/pull/7273#discussion_r4110064842
##########
shenyu-plugin/shenyu-plugin-proxy/shenyu-plugin-rpc/shenyu-plugin-dubbo/shenyu-plugin-apache-dubbo/src/main/java/org/apache/shenyu/plugin/apache/dubbo/proxy/ApacheDubboProxyService.java:
##########
@@ -80,6 +81,11 @@ public ApacheDubboProxyService(final
DubboParamResolveService dubboParamResolveS
* @throws ShenyuException the shenyu exception
*/
public Mono<Object> genericInvoker(final String body, final MetaData
metaData, final SelectorData selectorData, final RuleData ruleData, final
ServerWebExchange exchange) throws ShenyuException {
+ return Mono.defer(() -> invokeOnWorker(body, metaData, selectorData,
ruleData, exchange))
Review Comment:
Blocking (see review body): this hop moves the invocation to a
`boundedElastic` thread, but nothing moves the Dubbo `RpcContext` client
attachments with it. `ApacheDubboPlugin#doDubboInvoker` sets them on the
calling thread immediately before this call:
```java
RpcContext.getClientAttachment().setAttachment(CommonConstants.TIMEOUT_KEY,
dubboRuleHandle.getTimeout());
RpcContext.getClientAttachment().setAttachment(Constants.DUBBO_SELECTOR_ID,
selector.getId());
RpcContext.getClientAttachment().setAttachment(Constants.DUBBO_RULE_ID,
rule.getId());
RpcContext.getClientAttachment().setAttachment(Constants.DUBBO_REMOTE_ADDRESS,
...);
final Mono<Object> result = dubboProxyService.genericInvoker(...); //
ApacheDubboPlugin.java:69-77
```
`RpcContext.getClientAttachment()` is thread-bound and `invokeOnWorker`
never re-applies it (it only reads `RpcContext.getContext().getFuture()` at
line 115, which correctly stays on the invoking thread). Before this PR
`$invoke` executed eagerly on the same thread that wrote the attachments; now
it executes on the worker, so TIMEOUT_KEY / SELECTOR_ID / RULE_ID /
REMOTE_ADDRESS are not visible to the invocation. The new tests mock
`GenericService` and never read `RpcContext`, so they cannot catch it either
way.
Please either snapshot the attachments here and re-apply them inside
`invokeOnWorker` (clearing them afterwards from the pooled worker), or move
their population into the deferred stage, and add a test asserting they are
visible inside `$invoke`. Happy to flip to approve as soon as one of those is
in place.
--
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]
