`for await ... of ...` never end after upgrade NodeJS to 12.18.3 about multiparty

[dougwilson]

Hi @Mar-Ser I know this is an old issue, but I was able to track this down to a regression from the following commit in Node.js: nodejs/node@369b7c2 . This affects the async await function on streams in general. It seems to behave differently based on internal timings of events within Node.js, which is why it sometimes works, but mostly does not work. The good news is that it seems to not affect Node.js 13+, even this it is landed there. It was backported to Node.js 12.16.0 and likely is conflicting with some other behavior that is in Node.js 12 that is not in 13. Unfortunately it does not seem there is any possible way to fix it from this module.

[dougwilson]

Thank you, I will take a look. It looks like in 12.16.0 there were a bunch of changes around async iteration on event emitters, which could have introduced this change of behavior. In your example above, you are actually iterating over a built-in Node.js event emitter: a Passthrough stream.

[dougwilson]

Taking a look into it, I'm not able to reproduce with 2.18.3. When copying and pasting your exact program and following the instructions on 12.18.3, this is my console output:

$ node -v
v12.18.3

$ node index.js
1
2
4 Error: 1
    at Server.<anonymous> (C:\Users\dougw\GitHub\multiparty-231\index.js:13:15)
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
5

[mars-dlx]

This is my

[dougwilson]

Strange. I tried with various Node.js versions and all of them show all the numbers. Without the ability to reproduce the issue, there is nothing I can debug. Can you try following your steps above on a different machine just to see if it reproduces or not so we can narrow down which of our machines is the outlier?

[mars-dlx]

This is not related to my machine because in first time it was failed on CI... We (I and my colleague) spent a day trying to debug it. So, it already was reproduced in three different environments...

[mars-dlx]

Anyway, I will try to run this code on my home machine.

[dougwilson]

Hm, strange. After I sent that I tried on a laptop, which has never even had Node.js on it before, and worked just fine. Perhaps the issue is within some steps not provided above. I tried with Chrome and the file I uploaded was the index.js file itself. If you upload the index.js file through Chrome with the server, does it still happen?

[mars-dlx]

On home PC a result is the same:

$ node -v
v12.18.3


$ node index.js
1
2

Also, I tried to upload the index.js itself and the result also the same. And in all cases, I used Chrome, exclude CI.

[dougwilson]

Strange. There has to be something different between our methods... 🤔 . Maybe we need to wait for another member to come by and see if they can reproduce and debug through it. You're also always welcome to make a pull request if you can determine the issue, or even add this as a test to our test suite and open a PR with that added (it doesn't matter if it fails, etc.: it'll be a clean environment that gives us both a place to see it).

[dougwilson]

Here is another thing to try: I was going to make a reproduction with the PassThrough stream without multiparty, to determine if it was a Node.js issue or multiparty, but since I couldn't reproduce the issue, I am not able to verify if this demonstrates it or not as a reduced test case. Does this experience issues on your setup?

var stream = require('stream');

const pts = new stream.PassThrough();

const run = async () => {
  try {
    console.log(1);
    for await (const data of pts) {
      console.log(2);
      throw new Error('1');
    }
    console.log(3);
  } catch(err) {
    console.log(4, err);
  }

  console.log(5);
};

pts.write('test1');

run()
.then(() => { console.log('done!'); })
.catch((e) => { console.error(e); });

[mars-dlx]

No. This code passed.

[dougwilson]

Darn. So can you make a pull request to add this issue to our test suite to get it reproducing on our CI?

[mars-dlx]

Ok!

[mars-dlx]

Pipeline for 12.18.3 failed as expected by timeout. 10 and 14 passed. The old versions don't support for await. Going to exclude them later for this test.

[mars-dlx]

Our thoughts:
After throw or break, it exits from the for await cycle. But before exit, it tries to destroy the stream and waits until it happens. But something is going wrong, and it is never happening...

[dougwilson]

Yea, I agree. It seems to be getting stuck in the return() of the async iterater for some reason I'm trying to determine.

[mars-dlx]

@dougwilson Thank you!