fix: Make takeHeapSnapshot() work

[addaleax]

Instead of treating the total item count like an expected
byte length, wait for the JSON data to end before considering
the heap snapshot finished.

Fixes: #56

@jkrems This PR would, in its current state, introduce a dependency (that I wrote specifically for this use case)… I have no idea how okay that is. It might also make embedding into Node slightly trickier, so I’d also be okay with just copying in the dependency index.js as-is?

[bnoordhuis]

IMO, a better performing¹ solution is to make the V8 inspector emit an empty chunk or HeapSnapshotCompleted event. To have one part of the program emit JSON and have the other half validate is less than elegant.

I came up with a timer-based approach as an interim solution. Not elegant either but since the timing of the chunk events is pretty deterministic, it should be less race-y than it looks.

diff --git a/deps/node-inspect/lib/internal/inspect_repl.js b/deps/node-inspect/lib/internal/inspect_repl.js
index 937c1843d3..c04d13791b 100644
--- a/deps/node-inspect/lib/internal/inspect_repl.js
+++ b/deps/node-inspect/lib/internal/inspect_repl.js
@@ -900,26 +900,27 @@ function createRepl(inspector) {
         return new Promise((resolve, reject) => {
           const absoluteFile = Path.resolve(filename);
           const writer = FS.createWriteStream(absoluteFile);
-          let totalSize;
           let sizeWritten = 0;
           function onProgress({ done, total, finished }) {
-            totalSize = total;
             if (finished) {
               print('Heap snaphost prepared.');
             } else {
               print(`Heap snapshot: ${done}/${total}`, true);
             }
           }
+          let timer;
           function onChunk({ chunk }) {
             sizeWritten += chunk.length;
             writer.write(chunk);
-            print(`Writing snapshot: ${sizeWritten}/${totalSize}`, true);
-            if (sizeWritten >= totalSize) {
-              writer.end();
-              teardown();
-              print(`Wrote snapshot: ${absoluteFile}`);
-              resolve();
-            }
+            print(`Writing snapshot: ${sizeWritten}`, true);
+            clearTimeout(timer);
+            timer = setTimeout(onDone, 250);
+          }
+          function onDone() {
+            writer.end();
+            teardown();
+            print(`Wrote snapshot: ${absoluteFile}`);
+            resolve();
           }
           function teardown() {
             HeapProfiler.removeListener(

¹ Note how I didn't say 'performant', I hate that word.

[addaleax]

@bnoordhuis Yeah, I completely agree that that would be more elegant :)

[jkrems]

Thanks for looking into this! Looking at the devtools frontend source, I'm wondering if it would be enough to wait for heapProfiler.takeHeapSnapshot() to resolve and trust in the message order..? https://github.com/ChromeDevTools/devtools-frontend/blob/054e42ab6ab335488f3e948df9eec71a51ae020f/front_end/profiler/HeapSnapshotView.js#L1035-L1046

[bnoordhuis]

I can't tell how devtools collects the chunks, maybe their event loop integration is different.

In node chunks are emitted asynchronously and since there is currently no marker to indicate the last chunk, you can't really do better than my or Anna's approach.

[jkrems]

I still have to actually try it but from what I remember they do the same thing as the node implementation. Basically they use the response message to the .takeHeapSnapshot call ("promise of the RPC abstraction resolves") as a signal that all chunks have been received. Since messages are processed in-order, that should work here as well.

[bnoordhuis]

What you say about message ordering sounds right. The flow of control and data in V8 and Node.js is quite hard to follow so I can't vouch it's always true but we should probably treat it as a bug if it turns out it doesn't work that way. I filed #58.

[jkrems]

Closing as duplicate of #58. Thanks @addaleax and @bnoordhuis for jumping on this! :)