Imteyaz, Would you mind to take a look at pull request 3035 [1] I submitted just now?
Thanks, -Ian. 1. https://github.com/apache/incubator-dubbo/pull/3035 On Thu, Dec 20, 2018 at 4:41 PM Imteyaz Khan <[email protected]> wrote: > 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(); > >> >> > > } > >> >> > > } > >> >> > > } > >> >> > > > >> >> > > >> >> > >> > > >> > > >
