Parsestack refactor + tests to fix memory explosion #320
Conversation
|
One notable point I forgot to mention before is that the read-source-files-cache is on a per-parseStack basis, so the cached files are not reused between multiple parseStack calls. This is the same as it was before and my statement in #318 that
was incorrect. I believe this is our ideal default behavior, so as to ensure that in a long-running process we won't accumulate a large cache which ends up consuming a significant amount of memory. It does make for higher disk read load in many typical cases, though; I think a future option for whether to keep the cache around between calls could be useful for users who are more concerned about disk read load than about memory usage. |
| frame.context_line = lines[frame.lineno - 1]; | ||
| frame.post_context = lines.slice(frame.lineno, frame.lineno + LINES_OF_CONTEXT); | ||
| } catch (e) { | ||
| // anomaly, being defensive in case |
There was a problem hiding this comment.
do you want to set the properties to be empty strings /arrays here?
There was a problem hiding this comment.
nah, i think we'd rather not send them to the server at all; not sure what behavior diff would be but not sending them at all is what we do for native frames so i'll stick to that
| var uniqueFilesRead = filesRead.filter(function (filename, idx, arr) { | ||
| return arr.indexOf(filename) === idx; | ||
| }); | ||
| filesRead.length.should.equal(uniqueFilesRead.length); |
| stack.reverse(); | ||
|
|
||
| return stack.forEach(function (line, index) { | ||
| var frames = []; | ||
| var filesToRead = {}; |
There was a problem hiding this comment.
could new Object(null) here if you wanted idk
| var numFilesToRead = filenames.length; | ||
| return filenames.forEach(function (filename) { | ||
| fs.readFile(filename, function (readErr, file) { | ||
| if (!readErr) sourceFiles[filename] = file.toString().split('\n'); |
There was a problem hiding this comment.
should you wrap this line in a try catch, in case toString throws due to a weird file? to ensure that the callback always fires.
There was a problem hiding this comment.
if (!readErr) guarantees that file is a Buffer according to fs.readFile docs, and Buffers have toString (as linked) even if they're empty, so I'm not concerned
LewisJEllis
changed the title
Fixes #319, based on the insight in #318; thanks to @Asuka999 for the original insight + PR and @cveilleux for the out of memory report.
parseStackinto a more serial-procedural style with areadSourceFileshelperThe parseStack code in general needed some overhauling, so I went ahead and did that in addition to just fixing the concurrency control flow. It should hopefully be more robust and easier to follow now.
/cc @MaxBittker @benvinegar