Memory leak when loading images on the canvas in Node.js #5102
Comments
|
Did you try with StaticCanvas? just to reduce the area where the leak could be. You do not need to clear and dispose, dispose is taking care of everything, |
|
@asturur I tried just this with StaticCanvas and removed the canvas.clear but the behavior persisted. I also think the cause of the JS DOM leak, but I don't know how to clean it? I read that you can call window.close (). I tried to call the fabric.window.close (), but after this call, subsequent queries draw nothing. |
const http = require(`http`);
const {fabric} = require(`fabric`);
const JSON = `{"version":"2.3.3","objects":[],"background":"red"}`;
const server = http.createServer((req, res) => {
let {url} = req;
if (url === `/`) {
console.log(`Rss ${+process.memoryUsage().rss / 1024 / 1024 }MB`);
console.log(`Heap ${+process.memoryUsage().heapUsed / 1024 / 1024 }MB`);
const canvas = new fabric.StaticCanvas(null, {
width: 600,
height: 600
});
canvas.loadFromJSON(JSON, function () {
let pngStream = canvas.createPNGStream();
res.writeHead(200, {
'Content-Type': `image/png`
});
pngStream.on(`data`, function (chunk) {
res.write(chunk);
});
pngStream.on(`error`, function (exception) {
console.log(`Error on stream ${exception}`);
});
pngStream.on(`end`, function () {
canvas.dispose();
res.end();
});
});
if (global.gc) {
console.log('executing GC')
global.gc();
}
}
});
server.listen(8124, () => {
console.log(`Server is runing on http://localhost:8124`);
});with this minimal test case there is a small leak anyway, like 1 meg ever 100 requests. That i do not think is normal at all. Putting the GC outside the callback improve the situation anyway, i do think fabric maybe should do some explicit cleaning of resources on images first of all. |
|
Look i did this quick check
Changing this code does not solve the situation but makes it more stable, so the solution is somewhere around here. if i keep reload pressed, memory usage grow fast, but then if i wait a bit, and do a reload, the GC is able to clean stuff. not completely, something is still leaking |
|
also this code will probably crash on a browser. |
|
I added more cleanups, while it does not go back to the the original consumptions, is still recoveing. from 512 to 92 |
|
after a bit of rest, it cleaned more chunks an went back to 76, so maybe is not leaking substantial stuff anymore. But if you plan to handle many request concurrently, with JSON with many images and long async loadings, you may break the 1.5G limit easily |
|
@asturur you did a great job. I didn't know that I could access HTMLImageElementImpl and I just tried adding to the Image.dispose () also has this code. ['_originalElement', '_element', '_filteredEl'].forEach((function(key) {
var impl = fabric.jsdomImplForWrapper(this[key]);
if (impl && impl._image) {
impl._currentSrc = null;
impl._attributes = null;
impl._classList = null;
impl._image = null;
impl._eventListeners = null;
this[key] = undefined;
}
}).bind(this))It may look redundant and paranoid, but it's working. Rss 245.16015625MB
Heap 36.9376220703125MB
executing GCWithout your dispose code I saw this Rss 889.22265625MB
Heap 37.885032653808594MB
executing GCWe are going in the right direction. I think I still need to do dispose for canvas to clean up HTMLCanvasElementImpl. How to access it? |
|
@asturur After each request, the HTMLImageElementImpl remains in the heap |
|
the heap grow very little, is that a reference and the big part is then in
the rss?
how do you inspect that?
…On Mon, 16 Jul 2018, 04:20 0ro, ***@***.***> wrote:
@asturur <https://github.com/asturur> After each request, the
HTMLImageElementImpl remains in the heap
[image: image]
<https://user-images.githubusercontent.com/7969190/42748737-eeff0514-88e9-11e8-9306-9094e8b0f04b.png>
[image: image]
<https://user-images.githubusercontent.com/7969190/42748751-ff86e2da-88e9-11e8-9c12-11a5b574e6ea.png>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5102 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABI4QBmYq9ZcdMAvLZ0ZeX-E4DbN3_zrks5uHEy0gaJpZM4VO4fg>
.
|
Probably Yes.
Using the --inspect in node flag and chrome://inspect/ in the memory tab, I took a snapshot before and after request and then compared them
|
|
And I use supertest to generate requests const request = require(`supertest`);
const PORT = 8124;
const HOSTNAME = `127.0.0.1`;
const app = `http://${HOSTNAME}:${PORT}`;
const fs = require(`fs`);
let promises = [];
for (let i = 0; i < 100; i++) {
promises.push(request(app)
.post(`/`)
.type(`form`)
.send(``)
.expect(200)
.then((res)=> {
fs.writeFile(`bgs/bg${i}.png`, res.body, (err) => {
if (err) {
throw err;
}
});
}));
}
Promise.all(promises).then(() => {
console.log(`All`);
}); |
|
+1 to this. @0ro would you be able to paste more details on your solution? Might be nice for others coming to this thread. I have tried the following but perhaps I don't understand how you have applied your fix. Thanks to both you and @asturur on your awesome work with this issue. fabric.Image.dispose = function() {
console.log('Calling dispose for image');
var backend = fabric.filterBackend;
if (backend && backend.evictCachesForKey) {
backend.evictCachesForKey(this.cacheKey);
backend.evictCachesForKey(this.cacheKey + '_filtered');
}
this._originalElement = undefined;
this._element = undefined;
this._filteredEl = undefined;
['_originalElement', '_element', '_filteredEl'].forEach((function(key) {
var impl = fabric.jsdomImplForWrapper(this[key]);
if (impl && impl._image) {
impl._currentSrc = null;
impl._attributes = null;
impl._classList = null;
impl._image = null;
impl._eventListeners = null;
this[key] = undefined;
}
}).bind(this))
} |
|
@stewartmatheson The issue is not fix. memory is still leaks, but a little slower than before. HTMLImageElementImpl still remains in memory. |
|
I’m still seeing a huge increase in memory though so I don’t think the code I have posted is working correctly. Can you post your solution that applies the code correctly? |
|
@stewartmatheson Your code is correct, but I don't know how remove HTMLImageElementImp. There is still a link to it in memory. We clear its fields, but it remains in memory. We should try to remove its from memory. |
|
@asturur You know what to do with this? |
|
I tried to remove the src attribute of the image in order to clear it but got this error jsdom/jsdom#2150
|
|
@0ro How do you figure that its the
|
|
Hi, i just wanted to say that we should anyway verify what is the suggested way to get rid of JSDOM objects before do manual clenaning, it looks weird to me that we are the only one with this problem. I will open a pr with the changes i experimented with as soon as i have a bit of time. With this code the memory will increase anyway, what we can ask is that after all the async process is done, another forced GC will eventually erase everything. |
|
@asturur It looks like jsdom have had issues in the past that match up to our use case. jsdom/jsdom#784 contains a comment in there that suggests that if (typeof document !== 'undefined' && typeof window !== 'undefined') {
fabric.document = document;
fabric.window = window;
}
else {
// assume we're running under node.js when document/window are not present
fabric.document = require('jsdom')
.jsdom(
decodeURIComponent('%3C!DOCTYPE%20html%3E%3Chtml%3E%3Chead%3E%3C%2Fhead%3E%3Cbody%3E%3C%2Fbody%3E%3C%2Fhtml%3E'),
{ features: {
FetchExternalResources: ['img']
}
});
fabric.jsdomImplForWrapper = require('jsdom/lib/jsdom/living/generated/utils').implForWrapper;
fabric.nodeCanvas = require('jsdom/lib/jsdom/utils').Canvas;
fabric.window = fabric.document.defaultView;
DOMParser = require('xmldom').DOMParser;
}which means that the jsdom will always be a singleton instance. Fair enough as we don't have to pay the init cost each time however it's not getting GC'd. I think @0ro has hinted at this but like they said draws after calling close would not work as there is a singleton instance of jsdom. Is it reasonable for us to set |
|
OK so I have implemented a fix for the canvas loading. I am able to call |
|
it doesn t because is missing the code i pasted in some answer here. The
same code must be done for the canvas
…On Wed, 18 Jul 2018, 03:12 Stewart Matheson, ***@***.***> wrote:
OK so I have implemented a fix for the canvas loading. I am able to call
window.close() and then render again after assigning a new jsdom. However
it looks like there is another issue here. jsdom/jsdom#1665
<jsdom/jsdom#1665> is open and it speaks of the
JSDOM window hanging around after window.close() has been called. At this
point the work around solution of re-creating the jsdom instance will not
work due to this issue. I'm back at square one now. I think the best thing
to do is to understand why canvas.dispose() does not clean up the Images
as @0ro <https://github.com/0ro> has pointed out. Thoughts @asturur
<https://github.com/asturur> ?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#5102 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ABI4QP15rmG1e-BLwIxWA12gPfB1jjGGks5uHt_igaJpZM4VO4fg>
.
|
|
@asturur how are you doing with this problem? can I help you with something? I've spent a lot of time researching garbage heaps, but found nothing suspicious except |
|
https://github.com/fabricjs/fabric.js/pull/5142/files I made this PR that while it does not solve 100%, it stil better than the original situation by far. |
|
Thank you |
|
i m not sure we have a reproducible |
|
yes, I am trying with an image with 18 layers. 15 of them are image layer. I guess more the image layers, RSS increases faster. |
Version
2.3.3
Test Case
Steps to reproduce
Run the server script and make several requests to the address in the browser http://localhost:8124
Expected Behavior
process.memoryUsage().rss should not grow
Actual Behavior
When you create an http server on node.js that handles the requests in json and response the image created on the basis of these data. Each request to the server increases the process.memoryUsage().rss, but remains almost unchanged process.memoryUsage().heapUsed.
The text was updated successfully, but these errors were encountered: