Auto-closing async iterables

[mfulton26]

I see that there's been some discussion related to consistently implementing return() on Deno provided implementations of AsyncIterables in the past in #10577 and #14086. From what I can tell the latest thinking was to not remove the deprecation of it in FsWatcher so that FsWatcher would be consistent with other Deno provided async iterables.

Current Deno provided async iterables are from:

• Deno.readDir()
• Deno.watchFs() (Deno.FsWatcher)
• Deno.serveHttp() (Deno.HttpConn)
• Deno.listenDatagram (Deno.DatagramConn, unstable)
• Deno.listen() (Deno.Listener)
• Deno.listenTls() (Deno.TlsListener)
• have I missed any?

While looking at all these all (except for perhaps Deno.readDir()) would benefit from implementing return() to support auto-closing after iterating over the resource.

Instead of removing return() from FsWatcher to make it consistent with the other async iterables I suggest adding return() to the others.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days if no further activity occurs. Thank you for your contributions.

[mfulton26]

I am hesitant to make a PR for this due to previous discussions on which way to go but am willing to do so.

[iuioiua]

I want to second this. It's surprising that the other async iterables don't have .return(). I'd be happy to implement this if @mfulton26 no longer wants to. What do we think, @kt3k and @bartlomieju?

[kt3k]

We discussed this several times and concluded we don't add it. I don't remember the exact reasoning though.

Does anyone remember the reasoning?

cc @ry @piscisaureus @nayeemrmn

[kt3k]

BTW I guess adding .return to these async iterators at this point probably break many code in the ecosystem. I think it's not a good idea to implement this for 1.x

[nayeemrmn]

We discussed this several times and concluded we don't add it. I don't remember the exact reasoning though.

Does anyone remember the reasoning?

There's no such thing as Iterable::return(), there's only Iterator::return(). Iterable has a cleaner interface. It can be targeted by a for-loop but doesn't expose .next() methods and stuff like that. We want the return types for these methods to be simple Iterables with nice abstract substitutes for .next() and .return(), i.e. Listener::accept() and Listener::close().

FsWatcher::return() is legacy from when we returned IterableIterators. We moved away from these because they lock us in to a very baring interface that would inevitably have multiple methods to do the same thing. And the ones provided by Iterator would be worse for purpose e.g. .return() is async, .next() needs to return a wrapper object.

Even if there's an argument that an 'iterator' is more semantically appropriate for some of the APIs than an 'iterable', we would end up with an 'iterable iterator' anyway so it can be for-looped. So that's a moot point.

would benefit from implementing return() to support auto-closing after iterating over the resource.

@mfulton26 Could you show an usage example of this? I thought .return() is something you call explicitly when you're using the Iterator::next() instead of for-looping. I don't understand how auto-closing would work here.

[nayeemrmn]

Could you show an usage example of this? I thought .return() is something you call explicitly when you're using the Iterator::next() instead of for-looping. I don't understand how auto-closing would work here.

Actually I see, .return() is automatically called on break or error.

This is actually an implementation issue then -- we should make it so that the iterator returned by FsWatcher[Symbol.asyncIterable]() implements a return() which closes the resource. There's no meaning to adding a public FsWatcher::return().

There's still an argument against this though. .close() will error if called more than once so if it's something happening conditionally in the background, it could incur boilerplate to account for this.

[iuioiua]

~~Perhaps, we could deprecate .close() (as it's a non-standard method) in favour of .return(). The breaking change could then happen in Deno v2.~~ Edit: see below.

[nayeemrmn]

Perhaps, we could deprecate .close() (as it's a non-standard method) in favour of .return(). The breaking change could then happen in Deno v2.

.return() is not standard on Iterable, it's async, it's not consistent with non iterable resources.

[iuioiua]

Sorry, let me correct myself. Yes, these APIs should implement return() in their [Symbol.asyncIterable]() methods. close() should not be deprecated.

There's still an argument against this though. .close() will error if called more than once so if it's something happening conditionally in the background, it could incur boilerplate to account for this.

IIUC, we could adopt the approach outlined in https://github.com/denoland/deno/issues/10577#issuecomment-1073810140.