use cache in parseStack, prevent unnecessary reading of files

[suarezzz]

cache not used in parseStack

• use cache in parseStack, prevent unnecessary reading of files

[LewisJEllis]

Hi @Asuka999, thanks for the PR. I'm not sure if this has any behavior change, though. We already use the cache in parseStack here. Note the early return at the end of that branch, so the fs.readFile below won't occur if there's a cache hit.

Also, I'd rather not introduce any additional dependencies unless absolutely necessary. Including the promise package would increase our package + dependencies footprint from 280k to 524k and it isn't really needed.

[suarezzz]

Thanks for your reply.
I know that, but because JavaScript is asynchronous, forEach is completed before fs callback, which make cache never used.
Also, don't worry too much about size for Node side, and I do not want to increase to packages dependencies too, promise polyfill is for compatibility Node v0.8.0. if necessary, can write a function to resolve it.
I really want help improve the repo.

[LewisJEllis]

Ah, I see now - thanks for the clarification! I admittedly wasn't the most familiar with the parseStack method at first read, but now it makes sense. Existing behavior is that we won't read a file again if a prior parseStack call already read it (from a previous error capture), but if, within a single parseStack call, the stack has multiple frames from the same file, we will read that file multiple times. The concern, then, is that if we have a large source file with many function calls inside it, this could (unnecessarily) result in a significant disk read spike. This change will make each file be read just once, by the first frame to reference the file; subsequent frames will use it from the cache thanks to the .then.

I'm +1 on the change in general.

I still don't want to include the promise library, though; maybe the filesize concern isn't the best justification, but it came to mind first from the .npmignore housekeeping in #311. I must admit that the behavior of Promise.then is quite convenient for this read-or-retrieve-from-cache pattern, but it's not too hard to roll a little bit of control flow/memoization inline instead of bringing in an entire library. All of our current dependencies are something that we would have literally written ourselves for this library if they didn't already exist, but I don't see us writing an entire promise library (and its dependency, asap) just for the behavior of .then. If we were to bring in any sort of control flow library (promise or otherwise), there would be many other places we ought to utilize it, so I'd rather just avoid doing so. A lightweight thenable pattern would probably suffice here, or a two-pass "collect de-duped list of all files to read, then read them" approach.

I can merge this as-is and then follow on with a small refactor to eliminate the dependency, but if you want to do that part before merging feel free. Happy either way, just let me know.

[cveilleux]

I have tested this PR after running into out of memory issues ( see #319 ) and it has fixed the problem for me.

Although I agree that adding a major dependency like Promise is a bit heavy-weight.

[LewisJEllis]

Merged this, then going to follow-on to add the tests I mentioned in #319 and avoid the promise dep.

[suarezzz]

...ah.. so sorry that I still didn't think of a good way to solve it ..

[LewisJEllis]

No worries, I was happy to wait but since it's affecting the memory leak issue I wanted to get the shippable fix + confirming tests done ASAP.