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(); > >> > > } > >> > > } > >> > > } > >> > > > >> > > >> > > >
