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