WampCallee memory leak on InvocationData? about wampsharp HOT 6 OPEN

Can you try 22.5.1-develop-39?

from wampsharp.

Thanks, have tied it locally. It seems AsyncLocalRpcOperation.InnerInvokeAsync is awaiting the task to finish internally and doing the cleanup when CallResult is called. However at that stage, in the outer call at WampCallee.InvocationPattern the invocationData may or may not have been added into the mInvocations. It depends on which of the two finish first: the inner invocation task finishes or WampCallee.InvocationPattern. So it may still leaks in this situation below:

Proxy interface:

    public interface IEchoService
    {
        [WampProcedure("com.test.echo")]
        Task<string> Echo(string message, CancellationToken cancellationToken);
    }

This will still leak:

    public class EchoService : IEchoService
    {
        public async Task<string> Echo(string message, CancellationToken cancellationToken)
        {
            return await Task.FromResult(message);
        }
    }

or

        public Task<string> Echo(string message, CancellationToken cancellationToken)
        {
            Thread.Sleep(100); // anything takes some time but synchronously.
            return Task.FromResult(message);
        }

But this will not:

    public class EchoService : IEchoService
    {
        public async Task<string> Echo(string message, CancellationToken cancellationToken)
        {
            await Task.Delay(10, cancellationToken); //note this delay here.
            return await Task.FromResult(message);
        }
    }

I am thinking if we could somehow create the CancellationTokenSourceInvocation in WampCallee.InvocationPattern and save it into mInvocations, then start the real invocation and pass in the CancellationTokenSourceInvocation to hook up the cancellation. Opinions?

from wampsharp.

After some try, I have got something like this on my fork branch. You can try run the LeakTestOnCancellableCalls and use dotMemory to see the leak is stopped now. Please see what you think about it.

from wampsharp.

@bigbearzhu thanks for working on this and for your contribution! Unfortunately, it seems like your solution breaks api (methods no longer return IWampCancellableInvocation). I fixed this issue differently by adding a property IsInvocationCompleted to IWampCancellableInvocation that checks whether the invocation has been completed, and in that case, I remove the invocation from the dictionary. Can you please try 22.5.1-develop-41 and let me know if the problem is solved?

Thanks!
Elad

from wampsharp.

Thanks @darkl and sorry for not replying for quite some time. I have checked the change, it seems it would harder to leak now with the IsInvocationCompleted check. However, because the inner invocation is running in different thread, so there is no guarantee that the inner invocation would always complete or not complete at right spot before the outer WampCallee doing its IsInvocationComplete check. I have created some dummy code with a test to show you one scenario that it could still leak. You can run CallerDealerTests.LeakTestOnCancellableCalls locally to see how it leaks.

This scenario is where inner invocation already passed calling WampCallee.CleanupInvocationData but somehow only completed after the outer code finished all its IsInvocationCompleted check.

DotMemory snapshot showing leak:

On our local fork, we have implemented a fix a little bit different as we need a fix urgently for this issue. Feel free to see if it is ok to cherry pick. The main idea of the fix is to move the registration code into the AsyncLocalRpcOperation so that it is guaranteed to add/clean registrations in right order. Also we rolled back the breaking api changes so that it still returns IWampCancellableInvocation.

from wampsharp.

Hi @bigbearzhu, I know it has been a long time. I attempted to resolve both of your issues in 23.3.1-develop-45. I would appreciate it if you could test it and tell me if the issues are gone, whenever you find the time.

Thanks,
Elad

from wampsharp.