Khan, I know you may busy on other stuff, should I jump on to give it a fix?
Thanks, -Ian. On Thu, Dec 20, 2018 at 10:11 AM Ian Luo <[email protected]> wrote: > 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(); >> > > } >> > > } >> > > } >> > > >> > >> >
