Conversation
RpcClusterClientManager#scanUnusedClient removes a client from CLUSTER_MAP and then closes it, while DefClusterInvoker#getInvoker only checks whether the invoker is already cached. A request issued in that window is therefore sent through a client which is being torn down, and DefResponseFutureManager #closeClient fails all its in-flight requests with "Client(...) stop" (error code 999). That code is not in the circuit breaker list and the failure is even reported as a success, so the exception is simply propagated to the caller. - RpcClient#getPendingRequestCount(): new method, with a default implementation returning 0 to keep binary compatibility for existing implementations. It is implemented by DefRpcClient on top of DefResponseFutureManager#getPendingCount() (i.e. the size of the in-flight future map) and delegated by RpcClientProxy; - RpcClusterClientManager#isIdleTimeout: although the client is idle for too long, skip cleaning it when there are still requests in flight, so that it is re-checked in the next round instead of being closed; - DefClusterInvoker#getInvoker: document why the availability check is required on the fast path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
背景
RpcClusterClientManager#scanUnusedClient每 15 分钟扫描一次空闲客户端：先从CLUSTER_MAP摘除，再批量close()。而DefClusterInvoker#getInvoker的快速路径只判断缓存里有没有 invoker，不判断它是否还可用，因此存在一个窗口 —— 客户端已被判定空闲、正在被拆除，业务线程却拿到了它并发出了请求，最终在DefResponseFutureManager#closeClient里被判死刑：Client(...) stop（error code 999）。999 不在熔断列表内，失败还会被上报成成功，所以这个异常会直接透传到业务侧。
改动
scanUnusedClient在真正close()之前再做一次空闲判定：若期间lastUsedNanos已被业务线程刷新，说明该客户端正在被使用，把它放回CLUSTER_MAP并跳过关闭；同时把原来finally里无条件打印的 WARN 改为正常关闭时才打印，并补上关闭失败的catch，避免一个客户端关闭失败影响后续客户端；isIdleTimeout增加「在途请求」闸门：客户端虽已空闲超时，但仍有未完成的请求时跳过本轮清理，留到下一轮再判定，避免closeClient()把这些请求全部打成Client(...) stop；RpcClient#getPendingRequestCount()：接口提供default实现返回0，不破坏已有第三方实现的二进制兼容；DefRpcClient转发到DefResponseFutureManager#getPendingCount()（即futureMap.size()），RpcClusterClientManager.RpcClientProxy做委托；DefClusterInvoker#getInvoker快速路径上的isAvailable()检查补充注释（纯注释，无逻辑变化）。边界
getPendingRequestCount()是瞬时快照，判定与真正close()之间仍有极小窗口可能进来新请求；第 1 点的二次判定与快速路径的isAvailable()检查一起把该窗口收敛到最小。有持续流量时客户端本来也不该被回收，属于预期行为；
idle_timeout配 0 仍可从源头关闭清理。测试
trpc-core 新增 3 个用例、trpc-rpc-support 新增 1 个用例：
RpcClusterClientManagerTest#testScanSkipsClientWithInFlightRequest—— 空闲且有在途请求时不清理，请求结束后下一轮正常清理；RpcClusterClientManagerTest#testScanRescuesClientUsedAgainBeforeClosing—— 清理过程中被再次使用时放回CLUSTER_MAP、不被关闭；RpcClusterClientManagerTest#testScanContinuesWhenClosingFails—— 某个客户端关闭失败不影响其余客户端的清理；DefResponseFutureTest#testPendingRequestCount—— 在途请求数的透出。前 3 个用例在未合入本次修复时会失败（已实测），属于回归测试。
mvn -pl trpc-core clean test -Dtest='com.tencent.trpc.core.cluster..*Test,com.tencent.trpc.core.rpc..*Test' -Djacoco.skip=true
→ Tests run: 105, Failures: 0, Errors: 0, Skipped: 0
mvn -pl trpc-proto/trpc-rpc-support test
→ Tests run: 2, Failures: 0, Errors: 0, Skipped: 0
https://trpc-scan-unused-client.pages.woa.com/