Update pdf2svg.js #8461
Update pdf2svg.js #8461
Conversation
Changing file write to sync solves the problem. Looks like promise chaining with a file write at the end with out waiting for the write to happen causes the issue.
|
I wonder if there is a logical explanation to this fix -- node.js people recommend to use async callbacks.
callback chaining |
examples/node/pdf2svg.js
Outdated
| }); | ||
| } | ||
| }); | ||
| if(!fs.existsSync('./svgdump/')) { |
There was a problem hiding this comment.
Add a space after if, so that it becomes if (!fs.....
examples/node/pdf2svg.js
Outdated
| } | ||
| }); | ||
| if(!fs.existsSync('./svgdump/')) { | ||
| fs.mkdirSync('./svgdump/') |
There was a problem hiding this comment.
You need a semicolon at the end of the line.
examples/node/pdf2svg.js
Outdated
| if(!fs.existsSync('./svgdump/')) { | ||
| fs.mkdirSync('./svgdump/') | ||
| } | ||
| fs.writeFileSync('./svgdump/' + name + "-" + pageNum + '.svg', svgdump, 'utf-8'); |
There was a problem hiding this comment.
Change the double quotes to single quotes on this line. Moreover, the prints are now gone. Is there a way to add them back?
|
I'm also wondering about the explanation for this. I'm fine with the sync approach if that works, but I do not understand why the async approach does not work properly. |
|
I think the problem is not with the async calls but probably with when the call gets executed. Will continue working on it. FYI: |
|
I tried different different things but only the sync file write works. I think we need to consult a node export here to tell us what the issue is. From the previous link it seems like a memory leak problem. |
I tried adding callback to block a promise -- the same problem.
I'm worry it's just coincidence and the patch is just masking the real problem, which can surface later. |
|
Blocked by nodejs/node#13337 |
|
node.js experts say it is a memory leak, so switching to sync might only fix the issue only for this case and can reappear in the future. I recommend to wait until leak will be found. |
|
@yurydelendik take a look at this petkaantonov/bluebird#1068 it seems like an inherit problem in promises in node. |
|
@yshahin okay, pdf.js also have polyfill in compatibility.js, see if "forcing" it will help |
|
@yashsriv the yurydelendik/nodesegfault@2bb6f26 fixes the issue, so you are right it's due to Promises implementation. |
|
Blocked by nodejs/help#652 |
|
@yurydelendik how do you want to handle this? |
|
If it works with our promise polyfill and not with the native Node.js one, I'm not sure why the Node.js people claim the memory leak is on our side. Even if there is a memory leak, it shouldn't matter if you use sync/async writing. Personally I'm fine with the sync approach here to solve this issue, but I'll let @yurydelendik make the call. |
|
I'm closing this PR in favor of mine at #8540, because moving from async to sync is unnecessary. The reason that moving from async to sync "solves" the problem is that it forces Node.js to suspend execution until the file writing finishes. Consequently, pages are effectively processed sequentially. In this thread, it has been said that using our Promise polyfill solves the issue as well. I think that this is a mere coincidence. Normally promises are scheduled as a microtask, but with our polyfill it is scheduled using setTimeout, which causes the promise to resolve less quickly. Consequently, when the SVG generation process (with many "promises") finishes, some file writing requests have likely finished as well, allowing Node.js to reclaim the memory for the string that is passed to |
Changing file write to sync solves the problem.
Looks like promise chaining with a file write at the end with out waiting for the write to happen causes the issue.
Fixes #8419