please.js icon indicating copy to clipboard operation
please.js copied to clipboard

Memory leak

Open israellot opened this issue 9 years ago • 4 comments

Theres an issue on this line responses[response.id] = response.data; This array never gets cleared.

israellot avatar Jan 09 '17 20:01 israellot

There another memory leak on requests[this.id] = this; This line return $.extend(new Request(), obj); will trigger the insertion of another property on requests but it will never get removed.

israellot avatar Jan 09 '17 21:01 israellot

@israellot

Ideally this would have been easy to solve if Javascript had a Dictionary data structure which supported weak keys. That way requests and responses could've been garbage collected automatically if not in use.

It is important to keep the requests and responses persistent to support accessing jQuery and to add suport for chaining with promises in the other frame (undocumented). For instance, this makes the following possible:

please(iframe).$('.some-class').parent().parent().next().html().then(function (html) {
    console.log('html: ', html);
});

I'm not really sure how we could achieve the above without keeping request and response references (or after destroying them).

I guess we could write a manual gc that clears all old requests and responses older than some specific threshold (say 30-60 seconds) every specific interval of time. If some request or response is accessed after that duration, it could throw an error that says "Object expired".

What do you say?

fleon avatar Jan 13 '17 08:01 fleon

What I'm saying is that there as ghost references in the Requests due to a constructor that inserts references that won't get used and cleared. That was causing the References object to eventually grow so big it would crash the process as I was making several requests/sec with lots of data. I fixed that and I will make a pull request. I'm not using jquery requests on my test, just simple calls. In my opinion this library would be much more generic if it didn't rely on jquery. Accessing the dom across frames can be easily solved putting the dom concerning code on the requested frame and returning the result, as a normal request-response. Jquery attachment could be an optional plugin distributed in a separated file. You could rely on native promises or leave the choice up to the user of which promise library to use. I personally have bluebird on my projects and I would like to keep using it and not have jquery deferred. I will take some time and try to put these ideias into a proof of concept on top of your code and pull it as well. Thank you!

israellot avatar Jan 13 '17 09:01 israellot

Sure, please share the pull request, I will look into it.

On Fri, Jan 13, 2017 at 2:46 PM, Israel Lot [email protected] wrote:

What I'm saying is that there as ghost references in the Requests due to a constructor that inserts references that won't get used and cleared. That was causing the References object to eventually grow so big it would crash the process as I was making several requests/sec with lots of data. I fixed that and I will make a pull request. I'm not using jquery requests on my test, just simple calls. In my opinion this library would be much more generic if it didn't rely on jquery. Accessing the dom across frames can be easily solved putting the dom concerning code on the requested frame and returning the result, as a normal request-response. Jquery attachment could be an optional plugin distributed in a separated file. You could rely on native promises or leave the choice up to the user of which promise library to use. I personally have bluebird on my projects and I would like to keep using it and not have jquery deferred. I will take some time and try to put these ideias into a proof of concept on top of your code and pull it as well. Thank you!

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/wingify/please.js/issues/25#issuecomment-272396405, or mute the thread https://github.com/notifications/unsubscribe-auth/ACGhFfvzLG4j9yCCDAMppcW94juXkv-mks5rR0EDgaJpZM4LeuGp .

fleon avatar Jan 13 '17 09:01 fleon