feat: optional async ops

[kt3k]

Motivation

As described in #3694, it's necessary to end the program even when certain types of async ops are remaining in pending_ops list. The example of such op is op_poll_signal in #3610 (which not yet landed)

Solution

This PR enhances deno_core::Op::Async type. Now Op::Async has the flag blocks_exit along with its main future. The flag is used when checking the result of poll method of deno_core::Isolate and if the flag is false (= the op is optional), those ops are filtered out in calculation of Poll result, which means the program can exit with those optional ops still remaining.

---

closes #3694

[kevinkassimo]

Not so sure if this would have some impact under large amount of concurrent async op (iterating potentially thousands of pending ops for each poll).

When I did my impl in #2735 , I used a simple counter: whenever optional op added, increment; when done, decrement; when length of pending ops is same as optional ops counter amount, we know all the remaining are optional ones and can safely exit.

[kt3k]

I added a todo comment for it.

I agree that this could cause perf issue in future, but I think it's not a huge problem for a moment because there is only a few optional async ops (0 at this moment, and 1 if #3610 landed).

Do you think that we need to address it before landing this?

[bartlomieju]

@kt3k I'm not sure I understand why we need "optional" ops. Could you show some example?

[kevinkassimo]

@bartlomieju It is mostly for signals. We want to allow promises for signal handlers to be resolved when a signal is triggered, but that this promise should not block the process from exit if everything else is done, thus "optional".

[bartlomieju]

@kevinkassimo so say I have this script:

const sig = Deno.signal(Deno.Signal.SIGTERM);

for await (const _ for sig) {
  console.log("SIGTERM received!");
}

Should this op be optional? That means program would just quit after registering for signal.

[kevinkassimo]

@bartlomieju Treating signal ops as non-optional blocks exit, which I believe is not what people would expect with signal handlers.

But I do feel using async iterator pattern and have an optional async op is definitely strange, which was one of the reason when I originally did my old PR I implemented sigaction with listener styled API.

[kt3k]

@bartlomieju @kevinkassimo
I even think Bartek's example is ok to exit immediately because signal handling can never be a main purpose of the program. The user must have some other "main things" other than signal handling, and that "main thing" would block the program to exit.

And actually if we block the program to exit with for await (const _ for sig) {-loop, then we always need to call sig.dispose() to exit the for-await loop at the end of the main process. I think that is not what the users would expect.

[ry]

I like that this PR does not make any changes to the JS interface.

[piscisaureus]

I like that this PR does not make any changes to the JS interface.

Although sensible defaults nice, I would rather give the end user control over what ops are "optional" vs non-optional.
E.g. there are other ops that one might not want to wait for (e.g. a time-out timer, or a server that implements a debug interface).

[bartlomieju]

I think @kevinkassimo's implementation with 3-variant Op enum is a bit cleaner.

enum Op {
  Sync(..),
  Async(..),
  AsyncOptional(..)
}

I like that this PR does not make any changes to the JS interface.

Although sensible defaults nice, I would rather give the end user control over what ops are "optional" vs non-optional.
E.g. there are other ops that one might not want to wait for (e.g. a time-out timer, or a server that implements a debug interface).

Combined with above op we can simply add new field to JSON dispatch to handle such scenarios 👍

[bartlomieju]

I'd recommend using Op::Async and Op::AsyncOptional.

Op::AsyncOptional should be kept in Isolate.pending_optional_ops - which is the same type as Isolate.pending_ops.

[kt3k]

I created another version of this at #3721, which uses AsyncOptional instead of adding flag to Async. I'd like to avoid having Isolate.pending_optional_ops because if we have 2 FuturesUnordered list in isolate, it seems to me that it's difficult to poll these 2 futures list correctly.

[ry]

closed in favor of #3721