????????????
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"<ian....@gmail.com>;
????????: 2018??12??20??(??????) ????10:11
??????: "dev"<dev@dubbo.apache.org>;
????: 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 <khan.imte...@gmail.com> 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 <ian....@gmail.com> 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 <khan.imte...@gmail.com>
> > 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();
> > > }
> > > }
> > > }
> > >
> >
>
