Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

core.xhr seems to leak resources #301

Open
dborkan opened this issue Dec 15, 2015 · 6 comments
Open

core.xhr seems to leak resources #301

dborkan opened this issue Dec 15, 2015 · 6 comments

Comments

@dborkan
Copy link
Contributor

dborkan commented Dec 15, 2015

It is looking to me like core.xhr might be leaking resources.

Some background: we were noticing if we left uProxy logged into the GitHub social provider, our Chrome app would crash when left running overnight. We found that if we changed our polling interval (which makes several core.xhr requests) to be less frequent we could run longer before the app crashed, or if it was shorter it would crash faster (10 ms polling interval crashed in a few minutes).

We are currently trying XMLHttpRequest instead of core.xhr and it hasn't crashed yet. Also based on the timeline tab in the Chrome debugger, it appears to use less resources.

Here are some graphs from running uProxy in Chrome, logged into GitHub with polling every 100ms.

This graph shows that resource usage keeps increasing when we use core.xhr:
screen shot 2015-12-14 at 4 25 00 pm

And here is another graph where I replaced the main core.xhr call with XMLHttpRequest (there may be a few other core.xhr calls still in there). You can see it uses far fewer resources (head and listeners) over the same period of time:
screen shot 2015-12-14 at 4 16 56 pm

@dborkan
Copy link
Contributor Author

dborkan commented Dec 15, 2015

I also made a test Chrome app to try to replicate. This app just makes GET requests to google.com every 10ms.

Here is the graph for using core.xhr:
screen shot 2015-12-14 at 5 06 08 pm

But when I switched to XMLHttpRequest, for some reason Chrome's timeline doesn't show the memory or listeners.

Also here is the source code I'm using.. There are two versions here, one for core.xhr and one for regular XMLHttpRequest:
freedom-xhr.zip

@trevj
Copy link
Contributor

trevj commented Dec 15, 2015

Great report! We use this a little in uproxy-lib so it's great to have this flagged...

@ryscheng
Copy link
Member

Are we tearing down the provider after its no longer needed?

On Tue, Dec 15, 2015, 10:18 trevj [email protected] wrote:

Great report! We use this a little in uproxy-lib so it's great to have
this flagged...


Reply to this email directly or view it on GitHub
#301 (comment).

@trevj
Copy link
Contributor

trevj commented Dec 15, 2015

Probably not. Almost the only module we actually tear down properly is TCP sockets, in the SOCKS server:
https://github.com/uProxy/uproxy-lib/blob/master/src/net/tcp.ts#L53

@dborkan
Copy link
Contributor Author

dborkan commented Dec 15, 2015

No we haven't been doing .close() for core.xhr, possibly that is the problem.

Here is an example of us doing a core.xhr request: https://github.com/freedomjs/freedom-social-github/blob/1c257f1d752ab02a6190299fadc9bc37227a4e92/src/github-social-provider.js#L226. Some notes / questions on this:

  • This code looks almost identical to using a regular XMLHttpRequest, except it's created with freedom['core.xhr'](), and sync calls or property reads are now async (e.g. var status = xhr.status becomes xhr.getStatus().then(function(status) {...}))
  • If we were to call something like freedom['core.xhr'].close(xhr) here, would we have to do it in multiple places? In this example I'm thinking immediately before lines 236 and 239, and also in a handler for onerror? Or is there one place where we can close it? Can we still use the xhr object for methods like getStatus after it has been closed?

@trevj
Copy link
Contributor

trevj commented Dec 16, 2015

FYI, it was a pain to get close right for sockets -- think you probably did the right thing here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants