finish event fires too early

[blackmad]

Hi! 👋

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch avsc@5.7.7 for the project I'm working on.

I noticed that the "finish" event was being emitted on my stream before I had actually finished reading all the rows. I'm not sure if this is a consequence of using snappy with using an async callback to do its work. I observed this behavior empirically in a test, in production code, and via the use of logs. I think the issue is that the code passes through the finish event when it reads the last bytes, but not when it finishes decoding them.

I'm not sure if this is the right solution - I couldn't figure out how to make a Duplex stream swallow the "finish" event, so I added another "done" event that appears to more correctly finish.

Here is the diff that solved my problem:

diff --git a/node_modules/avsc/lib/containers.js b/node_modules/avsc/lib/containers.js
index cc71888..f990e0e 100644
--- a/node_modules/avsc/lib/containers.js
+++ b/node_modules/avsc/lib/containers.js
@@ -74,6 +74,9 @@ function RawDecoder(schema, opts) {

   this.on('finish', function () {
     this._finished = true;
+    if (this._remaining === 0) {
+      this.emit("done")
+    }
     this._read();
   });
 }
@@ -300,6 +303,11 @@ BlockDecoder.prototype._read = function () {
   }

   this._remaining--;
+
+  if (this._remaining === 0 && this._finished) {
+    this.emit("done")
+  } 
+
   var val;
   try {
     val = this._readValue(tap);

This issue body was partially generated by patch-package.

[blackmad]

actually ... I think this doesn't work because the finish event still flows through to other streams too early

[blackmad]

working on a reduced case repro for this, I spent a while in the avsc code trying to fix it and couldn't quite get there. it's something about overriding the _final function in the Duplex stream and not calling the passed callback until we know we're done, but I was struggling to know if there was another block left.

[mtth]

Hi @blackmad. Thanks for the thorough report. The finish event signals the end of the stream's writing side - it sounds like you are looking for end instead, which is emitted when there is no more data to be consumed from the stream.