Ian, Yes please, incorporate the fix. Will wait for your PR to review.
On Thu, Dec 20, 2018 at 2:01 PM Imteyaz Khan <[email protected]> wrote: > Ian, > You won't believe, but what a coincidence I just loggined to gmail, to > reply on this tread to say, can I assist you on this issue any way and saw > your email. Any thing is fine with me. > > Regarding the apporach, as you mentioned endCount needs to be called I > certinly agree with you, but I was more looking for simplified approach of > changing if condition check where first check using beginCount then > followed by max>0. In this case, begin count will be always set and before > the end of method endCount will reduce it. > > Please correct me if I am wrong here. > > On Thu, Dec 20, 2018 at 1:54 PM Ian Luo <[email protected]> wrote: > >> 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(); >> >> > > } >> >> > > } >> >> > > } >> >> > > >> >> > >> >> >> > >> >
