feat: optional async ops 2 #3721
Conversation
cli/ops/dispatch_json.rs
Outdated
| @@ -14,6 +14,9 @@ pub type AsyncJsonOp = | |||
| pub enum JsonOp { | |||
| Sync(Value), | |||
| Async(AsyncJsonOp), | |||
| // AsyncOptional is the variation of Async, which | |||
| // doesn't block the program exiting. | |||
| AsyncOptional(AsyncJsonOp), | |||
There was a problem hiding this comment.
"Optional" isn't very descriptive. Maybe we could call it something like AsyncNoKeepAlive ? AsyncIgnoredAtExit ? AsyncUnref ?
core/ops.rs
Outdated
| pub type OpResult<E> = Result<Op<E>, E>; | ||
|
|
||
| pub enum Op<E> { | ||
| Sync(Buf), | ||
| Async(OpAsyncFuture<E>), | ||
| // AsyncOptional is the variation of Async, which | ||
| // doesn't block the program exiting. | ||
| AsyncOptional(OpAsyncFuture<E>), |
core/ops.rs
Outdated
| fn poll(mut self: Pin<&mut Self>, cx: &mut Context) -> Poll<Self::Output> { | ||
| self.as_mut().0.as_mut().poll(cx) | ||
| } | ||
| } |
There was a problem hiding this comment.
Is this really necessary? Seems odd.
There was a problem hiding this comment.
Agree, I'm still advocating for Isolate.pending_optional_ops field.
Then in Isolate::poll:
...
loop {
inner.have_unpolled_ops = false;
#[allow(clippy::match_wild_err_arm)]
match inner.pending_ops.poll_next_unpin(cx) {
Poll::Ready(Some(Err(_))) => panic!("unexpected op error"),
Poll::Ready(None) => break,
Poll::Pending => break,
Poll::Ready(Some(Ok((op_id, buf)))) => {
let successful_push = inner.shared.push(op_id, &buf);
if !successful_push {
// If we couldn't push the response to the shared queue, because
// there wasn't enough size, we will return the buffer via the
// legacy route, using the argument of deno_respond.
overflow_response = Some((op_id, buf));
break;
}
}
}
#[allow(clippy::match_wild_err_arm)]
match inner.pending_ops_optional.poll_next_unpin(cx) {
Poll::Ready(Some(Err(_))) => panic!("unexpected op error"),
Poll::Ready(None) => break,
Poll::Pending => break,
Poll::Ready(Some(Ok((op_id, buf)))) => {
let successful_push = inner.shared.push(op_id, &buf);
if !successful_push {
overflow_response = Some((op_id, buf));
break;
}
}
}
}
...
There was a problem hiding this comment.
@bartlomieju But isn't that having the chance of starving?
There was a problem hiding this comment.
@kevinkassimo you might be right, then I guess using select would be in order.
Then final check returning Poll will still check if pending_ops is not empty ignoring pending_ops_optional
|
Actually I think given our current design it might be possible to do explicit |
I think that would be ideal |
I don't really like that solution - it requires deep changes to ops infra. At the moment when you dispatch op it's "black boxed" and you have no way to interact with it directly. Doing Although I see why it might be useful, I suggest to go with simpler approach for now (just optional ops, no way to control them after they're dispatched) to solve problem at hand (signal handlers). If valid use case arises for |
- rename AsyncOptional -> AsyncUnref - create Isolate.pending_unref_ops and use select
| * AsyncUnref is the variation of Async, which | ||
| * doesn't block the program exiting. | ||
| */ | ||
| /// AsyncUnref is the variation of Async, which doesn't block the program |
There was a problem hiding this comment.
Sorry. I misread your comment!
| match select(&mut inner.pending_ops, &mut inner.pending_unref_ops) | ||
| .poll_next_unpin(cx) | ||
| { |
This PR is the variation of #3715. This uses AsyncOptional enum variant instead of adding the flags.