Memory leak in RefreshAsync while using a CancellationToken? about madb HOT 7 CLOSED

Hi,

I had a quick look at the code and couldn't immediately spot a different code path based on the cancellation token being set (actually, the code never really checks for CancellationToken.None).

Did you run both tests in Debug or Release mode? Do you have an indication of how much memory you see being leaked?

from madb.

The increase is very slow but look here:

(TestCancellationToken(ct.Token))

(TestCancellationToken(CancellationToken.None))

The Screenshots are both in DEBUG. But i run in Release an got the same Values. I take two memory snapshots (while the memory use increases) and it looks like this:

And it looks like the "SocketExtensions"-Class could be the Problem.

I think the cancellationToken.Register (row 57 in SocketExtensions.cs) is the probleme. Because every call, the event get registered but never deregistered.

The CancellationTokenRegistration instance that can be used to deregister the callback.
(https://msdn.microsoft.com/en-us/library/dd321635.aspx)

Could btw. also a problem in AdbClient.cs row 296. But never test it^^

from madb.

Great catch, thanks! That does indeed seem to be the problem - CancellationToken.Register returns a CancellationTokenRegistration which should be disposed of, something we currently don't do.

I'm trying to find a Roslyn analyzer that catches these kind of exceptions, so I can add them to the project and make sure these kinds of issues don't come up again. It seems that Microsoft.Maintainability.Analyzers (which includes DoNotIgnoreMethodResults) together with SonarLint (which includes "IDisposables" should be disposed
) should do it, but I'm having some issues setting it up.

I'll keep you posted!

from madb.

Hi,

I've prepared a PR - #52, and an updated NuGet release, 2.1.0-beta301, that should address this issue.

Can you have a look and let me know if this resolves the issue?

Thanks!

from madb.

The Memory-Use is now as expected. Perfect :)

But is SonarLint needed for the enduser?

from madb.

Ah, my bad!

I've pushed 2.1.0-beta303 to NuGet which should fix this.

Thanks!

from madb.

Perfekt :)

Thx for this

from madb.