problem in ActiveLimitFilter

[beiwei30]

Quoted from Khan's email sent to [email protected]:

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

[khanimteyaz]

@beiwei30 I think this one has been fixed already. If yes, then should we close this? As I don't have the permission so I won't be able to close this issue.