[TIMOB-11671] Use static class instead of inner class to avoid memory leak #4862
Conversation
| } finally { | ||
| if (tiClient != null) | ||
| { | ||
| tiClient.client.setRedirectHandler(null); |
There was a problem hiding this comment.
I added this pull request to my version of the Titanium framework and sometimes got an NPE at this line while testing. Didn't analyze this too deeply but changed the code to
DefaultHttpClient httpClient = tiClient.client;
if (httpClient != null) {
httpClient.setRedirectHandler(null);
}
which seems to help. The NPE did not occur anymore after this.
|
If abort is called, client could be null, fixed the issue. Thanks to Philet. |
|
|
||
| public ClientRunnable(int totalLength) | ||
| public ClientRunnable(int totalLength, TiHTTPClient client) |
There was a problem hiding this comment.
This is not a static nested class. It is associated with an instance of TiHTTPClient and has direct access to that instance's methods and fields. So you don't need to pass the instance to the constructor.
ghost
assigned 
|
FR failed. Ran the test case but it can not run. The log is: I/TiAPI ( 3296): START TIME: Tue Dec 03 2013 00:27:54 GMT+0000 (GMT) |
|
Looks like the server http://meijiservice.cloudapp.net is no longer up |
|
Need a valid test case for FR. |
|
New test case is uploaded |
|
@hieupham007 Can you please take a look at this PR since Ping is out? |
|
Spoke with Vishal on this and we don't think using static class is going to prevent any memory leak problems. Will discuss with Sunil next week. |
|
First I do not think the heap usage as shown in the ticket has anything to do with the HTTPClient. It is more due to the XML DOM creation. There is no memory leak as far as I can see. Second I do not see how using static classes helps reduce memory usage. The Java GC is smart enough to resolve circular references and will collect the handler, request, response and thread when the HTTPClient is ready for Garbage Collection. If the idea is to explicitly remove references to these instance variables in the client then they should just be nulled out at the end of the run command in the finally block of the clientRunnable. See PR #5670 |
|
When I did the memory profiling using HPROF, I saw the httpclient being accumulated and the one that was holding was the inner classes. After the changing to static, I didn't see the httpclient in the heap. |
|
I understand the reasoning behind using static inner classes but the problem would arise only if the lifetime of the objects was different. I believe that the real reason the inner classes stayed around was due to the static variables validatingClient and nonValidatingClient which would keep the redirectHandler around and hence the TiHttpClient class. Can you check PR #5670 to see if the problem is resolved. |
|
I tested with #5670 and it seems to be working |
Change the inner classes to static classes so that parent is garbage collected even though inner class instance is referenced (in this case, the ClientRunnable and the thread)
https://jira.appcelerator.org/browse/TIMOB-11671