We cannot do this, beginCount and endCount need to be called when max == 0 too.
Thanks, -Ian. On Fri, Dec 21, 2018 at 1:12 AM 大步流星 <[email protected]> wrote: > 能不能这样： > Can you do this: > > > } finally { > RpcStatus.endCount(url, methodName, System.currentTimeMillis() > - begin, isSuccess); > if (max > 0) { > synchronized (count) { > count.notifyAll(); > } > } > } > > Change to the following: > > } finally { > if (max > 0) { > RpcStatus.endCount(url, methodName, > System.currentTimeMillis() - begin, isSuccess); > synchronized (count) { > count.notifyAll(); > } > } > } > > > > ------------------ 原始邮件 ------------------ > 发件人: "Ian Luo"<[email protected]>; > 发送时间: 2018年12月20日(星期四) 上午10:11 > 收件人: "dev"<[email protected]>; > > 主题: Re: ActiveLimitFilter some observation! Need your suggestion > > > > We cannot simply comment out 'endCount' in finally clause. If 'beginCount' > happens, then 'endCount' must be called. > > Thanks, > -Ian. > > On Wed, Dec 19, 2018 at 9:49 PM Imteyaz Khan <[email protected]> > wrote: > > > Ian, > > The approach you mentioned, I am certain it would works. I was thinking > > another approach > > > > if (!RpcStatus.beginCount(url, methodName, max) && max > 0) { > > } > > > > ... > > finally { > > //already existing endCount call. > > } > > > > Second approach I am mentioning because of simplification. Do let me know > > your thoughts on this. > > > > > > On Wed, Dec 19, 2018 at 6:39 PM Ian Luo <[email protected]> wrote: > > > > > Hi Khan, > > > > > > Thanks for pointing out this issue. I guess we could change logic to: > > > > > > if (max > 0 && !RpcStatus.beginCount(url, methodName, max)) { > > > ... > > > } else { > > > RpcStatus.beginCount(url, methodName); > > > } > > > > > > And we should do the same logic in ExecuteLimitFilter. Let me know your > > > opinion on this. > > > > > > Thanks, > > > -Ian. > > > > > > > > > On Wed, Dec 19, 2018 at 7:42 PM Imteyaz Khan <[email protected]> > > > wrote: > > > > > > > Hi All, > > > > I was trying to write UT for ActiveLimitFilter for > RuntimeException > > > > scenarion and below is my UT for the same > > > > > > > > ActiveLimitFilterTest.java > > > > > > > > @Test() > > > > public void testInvokeRuntimeExceptionWithActiveCountMatch() { > > > > URL url = > > > > > > > > > > URL.valueOf("test://test:11/test?accesslog=true&group=dubbo&version=1.1&actives=0"); > > > > Invoker<ActiveLimitFilterTest> invoker = new > > > > RuntimeExceptionInvoker(url); > > > > Invocation invocation = new MockInvocation(); > > > > RpcStatus count = RpcStatus.getStatus(invoker.getUrl(), > > > > invocation.getMethodName()); > > > > int beforeExceptionActiveCount = count.getActive(); > > > > try { > > > > activeLimitFilter.invoke(invoker, invocation); > > > > } catch (RuntimeException ex) { > > > > int afterExceptionActiveCount = count.getActive(); > > > > assertEquals("After exception active count should be same" > > > > , beforeExceptionActiveCount, > > afterExceptionActiveCount); > > > > } > > > > } > > > > > > > > > > > > Where I am expecting RpcStatus active count before call and after > > invoke > > > > should be same, irrespective of exceptional handling by > > ActiveLimitFilter > > > > (e.g. in this case it should be 0). UT showing me that after > > encountering > > > > exception it is not same, on my further investigation I found that > > > > > > > > > > > > - If there is no *actives (ACTIVE_KEY) *is set or if its value is > > less > > > > then *1* then it is always returning -1 (in UT active count), > which > > > > means there is more number of call can be possible then it is > > > > allowed(this > > > > is my interpretation , correct me if this is wrong). e.g. if > within > > a > > > > minute 10 call are allowed and if we encounter 5 *RuntimeException > > > *then > > > > in total we could landed up allowing 15 invoke. I am suspecting > this > > > is > > > > because *max *being *0* we don't increment active count of > > RpcStatus > > > > and then decrease it in finally block where we have not even have > > > > increase > > > > the count, because *max* is before the count increment > > > > > > > > if (max > 0 && !RpcStatus.beginCount(url, methodName, > > > max)) > > > > > > > > As a reference I am copying the current master branch code of > > > > ActiveLimitFilter.java. Would request to correct my understanding if > it > > > is > > > > wrong, and if you feel my observation is correct then I would can > > raise a > > > > PR for fixing this issue. > > > > *Note: UT is in my local I have not checked in into any branch. > > > > > > > > if (max > 0 && !RpcStatus.beginCount(url, methodName, max)) { > > > > long timeout = > > > > invoker.getUrl().getMethodParameter(invocation.getMethodName(), > > > > Constants.TIMEOUT_KEY, 0); > > > > long start = System.currentTimeMillis(); > > > > long remain = timeout; > > > > synchronized (count) { > > > > while (!RpcStatus.beginCount(url, methodName, max)) { > > > > try { > > > > count.wait(remain); > > > > } catch (InterruptedException e) { > > > > // ignore > > > > } > > > > long elapsed = System.currentTimeMillis() - > start; > > > > remain = timeout - elapsed; > > > > if (remain <= 0) { > > > > throw new RpcException("Waiting concurrent > > > > invoke timeout in client-side for service: " > > > > + invoker.getInterface().getName() + > > > > ", method: " > > > > + invocation.getMethodName() + ", > > > > elapsed: " + elapsed > > > > + ", timeout: " + timeout + ". > > > > concurrent invokes: " + count.getActive() > > > > + ". max concurrent invoke limit: " + > > > max); > > > > } > > > > } > > > > } > > > > } > > > > > > > > boolean isSuccess = true; > > > > long begin = System.currentTimeMillis(); > > > > try { > > > > return invoker.invoke(invocation); > > > > } catch (RuntimeException t) { > > > > isSuccess = false; > > > > throw t; > > > > } finally { > > > > RpcStatus.endCount(url, methodName, > > > > System.currentTimeMillis() - begin, isSuccess); > > > > if (max > 0) { > > > > synchronized (count) { > > > > count.notifyAll(); > > > > } > > > > } > > > > } > > > > > > > > >
