Memory leak fix

[seva-ask]

Hello!
I started to use RoboSpice library in my project and I liked its design and usability very much.
But I have faced a problem: when I execute many similar requests, I have a great memory leak after them. After investigating this problem I have found that memory leak is in mapPendingRequestToRequestListener field in SpiceManager class. Some listeners are not removed from this map. It is so because this field is IdentityHashMap, but remove method is called only for one of aggregated requests. So I changed code a little to remove all listeners after execution.

[stephanenicolas]

Hi @seva-ask ,

I am going to have a look at your code this week. It may take me a little time, but I think you found a bug I had the feeling of in #177 .

Thanks for this contribution. I will let you know about the status of this request as soon as I review it.

[stephanenicolas]

The feature has been added in commit 4e05130.

The review pointed to some interesting new features like a new event to distinguish when request is really added or just aggregated.

The new feature provides a more robust SpiceManager when many requests are triggered and are aggregated by the spice service (requestProcessor).

Thx seva-ask. I am going to deploy snapshots. if you could try the new snapshots and approve the new feature works for you, that would be awesome.

--UPDATE : snapshots are deployed.
Stéphane

[seva-ask]

Thank you. I will try new snapshots tomorrow and notify about results.

[stephanenicolas]

Thanks. That should be the last step before releasing version 1.4.7 . :)

Eager to hear your feedback.

[seva-ask]

As I can see, memory leak is fixed.
But new version contains another bug.
Now in SpiceManager exists this field:
private final PendingRequestHandlerSpiceServiceListener removerSpiceServiceListener;

private class PendingRequestHandlerSpiceServiceListener extends SpiceServiceAdapter {
    @Override
    public void onRequestAdded(CachedSpiceRequest<?> cachedSpiceRequest, RequestProcessingContext requestProcessingContext) {
        mapPendingRequestToRequestListener.put(cachedSpiceRequest, requestProcessingContext.getRequestListeners());
    }

    @Override
    public void onRequestProcessed(final CachedSpiceRequest<?> cachedSpiceRequest, RequestProcessingContext requestProcessingContext) {
        mapPendingRequestToRequestListener.remove(cachedSpiceRequest);
    }
}

And this listener adds a request to map is each instance of SpiceManager.
I mean that I have one instance of SpiceManager per activity.
And actually if there is more than one instance of SpiceManager, which is bound to SpiceService, than added request will appear in these maps in all such SpiceManager instances.
This map is used to remove pending requests when SpiceManager is getting stopped. So when I stop it, all pending requests will be removed, although some of them were created in another SpiceManager and another activity.

[seva-ask]

At the moment I don't know how to fix this bug quickly, but I am thinking about it. Maybe you will find the solution faster.

[seva-ask]

I suggest returning to previous scheme of adding and removing listeners in spicemanager. So I made one more pull request with this changes. #186

[stephanenicolas]

Hi Ivanov,

really nice bug again. I will solve it, don't worry, altough I got
absolutely no idea now about the way I will. But it should be around
tomorrow.
How do you get so precise data about what happens in RS ? That's very
interesting, you got very accurate bugs and pinpoint exactly to the
problems.
Do you run RS in debug ?

S.

2013/9/2 Vsevolod Ivanov [email protected]

At the moment I don't know how to fix this bug quickly, but I am thinking
about it. Maybe you will find the solution faster.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/182#issuecomment-23665467
.

Stéphane NICOLAS,
OCTO Technology
Développeur & Consultant Android / Java
..........................................................
50, Avenue des Champs-Elysées
75008 Paris
+33 (0)6.26.32.34.09
www.octo.com - mobilite.octo.com
blog.octo.com - www.usievents.com
...........................................................

[stephanenicolas]

Quickly thinking about it :

• we could add a spicemanager to the requestprocessingcontext and check if
  it is == this in spicemanager when we receive an event
• we could remove the request from the map of request to launch when
  request is added, and cancelled and aggregated and add it to the map of
  pending request if and only if was contained in the previous map
• we could return something when adding a request to the spiceservice, like
  a boolean

the third way seems poor to me as may need other information as well in the
future and a boolean doesn't hold that much information

the first way looks interesting but I fear about memory leaks (as always).
The second way seems to be more difficult to achieve in a full and complete
way.

We should also add a test to get sure the bug you just found doesn't come
back.

If you feel like fixing it yourself, then I would appreciate the test and a
reasoning/justification for your preferred solution.

Stéphane

2013/9/2 Stéphane Nicolas [email protected]

Hi Ivanov,

really nice bug again. I will solve it, don't worry, altough I got
absolutely no idea now about the way I will. But it should be around
tomorrow.
How do you get so precise data about what happens in RS ? That's very
interesting, you got very accurate bugs and pinpoint exactly to the
problems.
Do you run RS in debug ?

S.

2013/9/2 Vsevolod Ivanov [email protected]

At the moment I don't know how to fix this bug quickly, but I am thinking
about it. Maybe you will find the solution faster.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/182#issuecomment-23665467
.

Stéphane NICOLAS,
OCTO Technology
Développeur & Consultant Android / Java
..........................................................
50, Avenue des Champs-Elysées
75008 Paris
+33 (0)6.26.32.34.09
www.octo.com - mobilite.octo.com
blog.octo.com - www.usievents.com
...........................................................

Stéphane NICOLAS,
OCTO Technology
Développeur & Consultant Android / Java
..........................................................
50, Avenue des Champs-Elysées
75008 Paris
+33 (0)6.26.32.34.09
www.octo.com - mobilite.octo.com
blog.octo.com - www.usievents.com
...........................................................

[seva-ask]

Yes, of course I run RS under debugger. I have some problems with it, because I use Android Studio and gradle projects which are not compatible with maven. So I have created gradle project for RS and attached sources from repository to it. But in this scheme I don't have test projects, so sometimes I can open original project in IDEA and run tests.
Also I inspect hprof dumps when I meet memory leaks.

The main problem that I see in this bug and around it - almost everything in robospice is operating with requests, but in my case I operate with listeners. Suppose we have two instances of SpiceManager and we are executing the same request in them with different listeners. Every callbacks are operating with Set, but listeners in this set may have come from different SpiceManagers. So if we are stopping one of SpiceManager instances, we actually want that request executing will continue and in the end listeners, that have come from all other SpiceManager instances will be run, and from stopped instance will not.
Maybe we should introduce new class that aggregates RequestListener and SpiceManager from where it came from.
So on I think that 1 and 3 ways will not work because they don't match Listener to SpiceManager.
Second way should work.
In #186 I implemented some solution that is simpler and should work too.

[stephanenicolas]

Hi Vselod,

thanks for sharing those ideas and for the pull request. The only thing
that bugs me in this request is that the set of pending requests is going
to grow and the get shrinked down.

I am wondering if there couldn't be a shared form of aggregation both at
the spiceservice level and at the spicemanager level. That would be a
better design. It would prevent, in spicemanager, both maps (requests to
launch and pending) to grow in case of repetitive requests.

Don't you think so ?

2013/9/3 Vsevolod Ivanov [email protected]

Yes, of course I run RS under debugger. I have some problems with it,
because I use Android Studio and gradle projects which are not compatible
with maven. So I have created gradle project for RS and attached sources
from repository to it. But in this scheme I don't have test projects, so
sometimes I can open original project in IDEA and run tests.
Also I inspect hprof dumps when I meet memory leaks.

The main problem that I see in this bug and around it - almost everything
in robospice is operating with requests, but in my case I operate with
listeners. Suppose we have two instances of SpiceManager and we are
executing the same request in them with different listeners. Every
callbacks are operating with Set, but listeners in this set may have come
from different SpiceManagers. So if we are stopping one of SpiceManager
instances, we actually want that request executing will continue and in the
end listeners, that have come from all other SpiceManager instances will be
run, and from stopped instance will not.
Maybe we should introduce new class that aggregates RequestListener and
SpiceManager from where it came from.
So on I think that 1 and 3 ways will not work because they don't match
Listener to SpiceManager.
Second way should work.
In #186 #186 I
implemented some solution that is simpler and should work too.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/182#issuecomment-23682899
.

Stéphane NICOLAS,
OCTO Technology
Développeur & Consultant Android / Java
..........................................................
50, Avenue des Champs-Elysées
75008 Paris
+33 (0)6.26.32.34.09
www.octo.com - mobilite.octo.com
blog.octo.com - www.usievents.com
...........................................................

[seva-ask]

Sorry, I don't understand you clear enough, can you please try to explain again?
In my request I don't increase number of requests which are stored in maps in SpiceManager. I made that only listeners which were executed by current instance of SpiceManager are stored in it.
How can we not store listeners and requests in SpiceManager? If so, we will not be able to cancel them explicitly.
By the way, most memory leaks in my case are caused by listeners and not by requests. For listeners I use anonymous classes which keep implicit reference to view, so view can't be gc'ed.

[stephanenicolas]

Snapshot has been deployed on sonatype and repo branch.

[seva-ask]

Thank you for fixing. We are comparing memory usage of two variants of fixes. I am going to post results here tomorrow.

[stephanenicolas]

Fine, thanks.

Would you be interested in writing a small wiki entry on the github repo to
help us tracking memory leaks ?

Stéphane

2013/9/20 Vsevolod Ivanov [email protected]

Thank you for fixing. We are comparing memory usage of two variants of
fixes. I am going to post results here tomorrow.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/182#issuecomment-24781501
.

Stéphane NICOLAS,
OCTO Technology
Développeur & Consultant Android / Java
..........................................................
50, Avenue des Champs-Elysées
75008 Paris
+33 (0)6.26.32.34.09
www.octo.com - mobilite.octo.com
blog.octo.com - www.usievents.com
...........................................................

[seva-ask]

As I can see, new build works well.
But on Sony Tablet S our testers could reproduce OutOfMemory again, and I can get this device and investigate the problem only next week. I suppose that this problem is not caused by robospice but I'm not sure now.
I will try to write some text for wiki. When it is ready, I will inform you.

[stephanenicolas]

Hi vsevolod,

I am impressed by the measures you deploy to test your app. I would be
interested to know which app you re talkin' about.

I think I will release 1.4.7 soon. It's already late a bit. I can release a
bug fix quickly if there us any problem left yo correct.

S.
Le 20 sept. 2013 20:06, "Vsevolod Ivanov" [email protected] a
écrit :

As I can see, new build works well.
But on Sony Tablet S our testers could reproduce OutOfMemory again, and I
can get this device and investigate the problem only next week. I suppose
that this problem is not caused by robospice but I'm not sure now.
I will try to write some text for wiki. When it is ready, I will inform
you.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/182#issuecomment-24829491
.

[seva-ask]

It is a good idea to release 1.4.7.
We are developing this app: https://play.google.com/store/apps/details?id=ru.stream.droid
Maybe you can't download it because it is only for Russia, but I'm not sure. If you are interested, I can send you .apk or we can invent another way for giving it to you.
Version, that is in market now, is written in Xamarin (http://xamarin.com/monoforandroid). Our company had been developing apps with this framework for two years but now we faced great problems with it. In MonoForAndroid garbage collection is implemented in very strange way, so it has great lags, pauses and even many crashes. I tried to explain problem to their support, attached some simple projects with great pauses but they haven't suggested any suitable solutions, they only suggested not to hold references for UI objects, what can't be done in projects with complex UI. So we had no other way except full rewriting our app in Java without Xamarin. Version written in Java is not published in market yet, we are going to do it in one-two months.
When we were writing in C# with Xamarin, we had to develop some kind of library for executing requests like robospice, but we had to do it crossplatform to work on iOS too. It was really bad idea because requests were not connected to activity lifecycle, but I got some experience in such libraries and in solving problems with random crashes and memory leaks.
One more thing: our app may become preinstalled into new Sony devices in Russia, so it has to pass Sony's QA. It is very strict, for example, app should have no crashes and anrs during 5 hours of special monkey test.

[stephanenicolas]

Hi Vselod,

thanks for these informations. I am eager to see your app getting accepted
at Sony's QA. I have no doubt you can reach this level of robustness when I
see the way you found the last bugs in RS :) If you need any additional
patch or feature in RS, please let me know. RS has already passed quite a
few audits of performance, robustness and security.

You are right a cross platform framework is more than often disappointing,
I don't think it's really possible to get things as well done as with a
native app. There are quite a lot of subtlities to take into account when
developping for a mobile OS...

I have always wanted to add a section to RS about apps using it. Would you
mind to let me know when you will pubilsh the app so I can add its icon ?

The app looks really nice and well done by the way.
(I could see it on the store and it looks possible to install it, I stay in
France).

Greetings,
Stéphane

2013/9/21 Vsevolod Ivanov [email protected]

It is a good idea to release 1.4.7.
We are developing this app:
https://play.google.com/store/apps/details?id=ru.stream.droid
Maybe you can't download it because it is only for Russia, but I'm not
sure. If you are interested, I can send you .apk or we can invent another
way for giving it to you.
Version, that is in market now, is written in Xamarin (
http://xamarin.com/monoforandroid). Our company had been developing apps
with this framework for two years but now we faced great problems with it.
In MonoForAndroid garbage collection is implemented in very strange way, so
it has great lags, pauses and even many crashes. I tried to explain problem
to their support, attached some simple projects with great pauses but they
haven't suggested any suitable solutions, they only suggested not to hold
references for UI objects, what can't be done in projects with complex UI.
So we had no other way except full rewriting our app in Java without
Xamarin. Version written in Java is not published in market yet, we are
going to do it in one-two months.
When we were writing in C# with Xamarin, we had to develop some kind of
library for executing requests like robospice, but we had to do it
crossplatform to work on iOS too. It was really bad idea because requests
were not connected to activity lifecycle, but I got some experience in such
libraries and in solving problems with random crashes and memory leaks.
One more thing: our app may become preinstalled into new Sony devices in
Russia, so it has to pass Sony's QA. It is very strict, for example, app
should have no crashes and anrs during 5 hours of special monkey test.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/182#issuecomment-24860765
.

Stéphane NICOLAS,
OCTO Technology
Développeur & Consultant Android / Java
..........................................................
50, Avenue des Champs-Elysées
75008 Paris
+33 (0)6.26.32.34.09
www.octo.com - mobilite.octo.com
blog.octo.com - www.usievents.com
...........................................................

[seva-ask]

Thank you very much for your tenderness and warm words.
Of course I will write you when we release our app, section with app using RS is a very nice idea.