Node: fs.promises.opendir should implement ECMAScript async iterator protocol 25.1.1.4

[haraldrudell]

Hi all,

The return value from fs.promises.opendir Node.js v14.4.0 appears to be an async iterator but does not actually implement the AsyncIterator interface properly 25.1.1.4

The below code shows that an aborted for await loop will not invoke the cleanup method .close(), that should be called .return() ECMAScript 2021

IMPACT: for await does not work as intended, boilerplate get-around code is required

I believe the fix is somewhere in lib/fs.js

/*
© 2019-present Harald Rudell <[email protected]> (http://www.haraldrudell.com)
All rights reserved.

node src/13-07-iteration-statements/test-fs.Dir.js
#0[object Object] iterator properties: return: undefined close: [Function: close]
#1[object AsyncGenerator] iterator properties: return: [Function: return] close: undefined
I am the returnest
*/
import {promises} from 'fs'
const {opendir} = promises
import {inspect} from 'util'

const deb = console.error
const thisIsNotAnError = new Error('this is not an error')
let i = 0

fn(opendir)
  .then(() => fn(asyncIterator))
  .catch(e => deb('error handler:', e) + process.exit(1))

async function fn(generatorFn) {
  const s = `#${i++}`
  const dir = await generatorFn('.')
  const {return: rfn, close} = Object(dir)
  deb(`${s}${dir} iterator properties: return: ${inspect(rfn)} close: ${inspect(close)}`)
  const cls = (dir.close || dir.return).bind(dir)
  dir.close = (...args) => deb('I am the closest') || cls(...args)
  dir.return = (...args) => deb('I am the returnest') || cls(...args)

  return forAwait(dir).catch(e => {
    if (e !== thisIsNotAnError) throw e
  })
}

async function forAwait(asyncIterator) {
  for await (const dirent of asyncIterator) throw thisIsNotAnError
}

async function* asyncIterator() {
  yield 1
}

[bnoordhuis]

The relevant code is here:
https://github.com/nodejs/node/blob/bff7de3b43510232f205f11c199ee54cb5b803ec/lib/internal/fs/dir.js#L199-L211
Note how it's implemented as an async generator function that calls an internal method to clean up, not the public fs.Dir#close() method that you're monkey-patching.

It's working as expected as far as I'm concerned unless you have a compelling reason why node should behave differently.

[ronag]

Agree with @bnoordhuis. I don't see a problem here.

[haraldrudell]

Hi,

It does work. However it is not the way ECMAScript 2021 intended it to be

• It is not compatible with for await which is the language construct most commonly used

• It does not implement the AsyncIterator interface of ECMAScript

I think there should be a return method implemented in a way that makes for await decide to invoke it

Apparently this could reduce code lines by 92%

It would turn your code into a one-liner

[ronag]

• It is not compatible with for await which is the language construct most commonly used

In what way exactly?

• It does not implement the AsyncIterator interface of ECMAScript

In what way exactly? I don't think you can implement an AsyncIterator in a more correct way than by using an async generator.

I think there should be a return method implemented in a way that makes for await decide to invoke it

The finally clause is basically the return statement when using an async generator.

Apparently this could reduce code lines by 92%
It would turn your code into a one-liner

How?

[bnoordhuis]

@haraldrudell I'm going to close this but let me know if you feel there is reason to reopen.

[haraldrudell]

I assume that @bnoordhuis knows ECMAScript well enough to understand what the problem is here

I also know that this is happening due to the harassment against former Zynga employees whose investors also held Joyent is still alive and well in 2020. I used to be harassed on premise at the Node.js meetings including by @isaacs and those people that used to be with Voxer that no do enterprise Node

There was a similar issue with fs nanosecond in which @bnoordhuis acted inappropriately. Node 14, the api I asked for exists

@bnoordhuis actions are not right for Node.js. I contend he is closing his @haraldrudell issue not objectively considering the merits of the issue

@ronag needs to study

[ronag]

@haraldrudell Why not open a PR that fixes the issue and a corresponding test? Maybe that will help us understand. As it is now none of see a problem.

[benjamingr]

@haraldrudell even if you were technically right on the technical bit (which you're not and Robert was trying to explain) it is entirely inappropriate to attack project collaborators. Ben does not owe you support, he was trying to be helpful and aid you with an issue _and you attacked him_. If I were you I'd ponder on that, apologize and move on. The issue was closed after a few days are inactivity. Ben was entirely appropriate in closing it and he was doing the charitable thing in helping keep the issue tracker tidy.

Further abuse on the issue tracker will mean interaction limits on the tracker. For what it's worth, I have never worked for or with Joyent, Voxer or Zynga and I would have acted the same. I also find pinging Isaac (whom is no longer involved with the project in a day-to-day capacity afaik) very poorly mannered.

@bnoordhuis I encourage you to lean more into the support and moderation mechanisms. I know you're a responsible competent adult and have been putting up with this sort of abuse longer than most - but you really don't and shouldn't have to put up with this.

As for the technical bits: the point Robert was trying to illustrate is this:

{
async function* foo() {
  try {
    yield 1;
  } finally {
    console.log('this gets called when `.return` is called on the generator');
  }
}
let iterator = foo();
iterator.next(); // progress to the first yield
iterator.return(); // console.log happens here
}

And with for await:

{
async function* foo() {
  try {
    yield 1;
  } finally {
    console.log('this gets called when `.return` is called on the generator');
  }
}
(async () => {
  for await (const item of foo()) {
    break;
  }
})();
}

When the break happens, the console.log gets called (it's a finally, so obviously it also happens on normal completion).

[haraldrudell]

The fact here is that the asyncIterator opendir appears to have been rewritten several times. It does not currently comply with the latest on for await. This can be fixed. Unimportant but true. No 14-line example that happens to work will change the fact that it is meant to be a one-liner

I am of the opinion that the Node,js project has several problems by not supporting host apis, not supporting unique features of certain file systems, not deprecating language features that should no longer be used and that several people allowed to make commits do not write 2015+ ECMAScript. This is where I like to help

What I am contending is that @bnoordhuis treats me differently because I am @haraldrudell
If he is doing that, he should not have a position of authority even if he offers to do it for free
I think it is likely that he has more programming experience than I have, but it appears to me that I more quickly absorb changes to the language. I do not think I have ever met him, I think he is in the Netherlands

@ronag, who I do not know, resembles the pattern used for harassment with several discrediting posts that are being thumbed-up by someone else. although I do not know if he is guilty as charged, it does look like it. Often these people come out of some hack-reactor style program and are quite ignorant, like the @ronag questions above

When I post anywhere under my own name, this happens because “they” search for it and respond with discrediting posts. Last week they locked my Twitter account, yesterday they managed to robocall my true mobile number which is a problem because in the US, you can track anyone in real time if you have their mobile number

“they” here is known to be linked to Reid Hoffman, now on the board of Microsoft, Mark Pincus and Michael Moritz of Sequoia. “they” could even place Jack Dorsey on Stevenson Street when surveillance showed I was going to be there. Twitter‘s board knows. I have worked for all three and this began as far back as 2006

The event with @isaacs was in the Joyent office. Joyent had a separate viewing room for some unknown parties, and isaacs was in it. During the event they flashed the spotlights and announced “Zynga is in the house” over the loudspeakers, meant to make me aware of that I cannot get away from Pincus anywhere. They did the same several times at K&L Law Firm 4 Embarcadero, even using the same cameraman

After my interview at Voxer, I was followed home in a vehicle pursuit leading to several subsequent burglaries that the sfpd refused to investigate because they entered using the landlord key

So, tell me he is innocent. Insult my intelligence

[benjamingr]

Randomly in case other people run into this and need to decorate an async generator, the "correct" way would be:

(async () => {
    async function* gen() { 
        try { 
            yield 1;
        } finally {
            console.log('in finally');
        }
    }
    const iter = gen()[Symbol.asyncIterator]();
    iter.return = () => console.log('in iterator return');
    // In this case, only the original `.return` is called due to the spec 
    for await (const entry of iter) {
        console.log(entry);
    }
    async function* wrap(iter) {
        try {
            yield* iter;
        } finally {
            console.log('You can put additional code you want to run here');
        }
    }
    const wrapped = wrap(gen()[Symbol.asyncIterator]());
    // here, both logs happen, using native ECMAScript features
    // note this has nothing to do with Node.js and everything to do with the 
    // ECMAScript spec
    for await (const entry of wrapped) {
        console.log(entry);
    }
})();