-
Notifications
You must be signed in to change notification settings - Fork 5.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: optional async ops #3715
feat: optional async ops #3715
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -81,5 +81,5 @@ pub fn op_fetch( | |
Ok(json_res) | ||
}; | ||
|
||
Ok(JsonOp::Async(future.boxed())) | ||
Ok(JsonOp::Async(future.boxed(), true)) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -511,9 +511,11 @@ impl Isolate { | |
let op_id = 0; | ||
Some((op_id, buf)) | ||
} | ||
Op::Async(fut) => { | ||
Op::Async(fut, blocks_exit) => { | ||
let fut2 = fut.map_ok(move |buf| (op_id, buf)); | ||
self.pending_ops.push(fut2.boxed()); | ||
self | ||
.pending_ops | ||
.push(Pin::new(Box::new(PendingOp(fut2.boxed(), blocks_exit)))); | ||
self.have_unpolled_ops = true; | ||
None | ||
} | ||
|
@@ -742,8 +744,8 @@ impl Future for Isolate { | |
inner.check_promise_errors(); | ||
inner.check_last_exception()?; | ||
|
||
// We're idle if pending_ops is empty. | ||
if inner.pending_ops.is_empty() { | ||
// We're idle if all pending_ops have blocks_exit flag false. | ||
if inner.pending_ops.iter().all(|op| !op.1) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
Poll::Ready(Ok(())) | ||
} else { | ||
if inner.have_unpolled_ops { | ||
|
@@ -814,6 +816,7 @@ pub mod tests { | |
|
||
pub enum Mode { | ||
Async, | ||
AsyncOptional, | ||
OverflowReqSync, | ||
OverflowResSync, | ||
OverflowReqAsync, | ||
|
@@ -834,7 +837,18 @@ pub mod tests { | |
assert_eq!(control.len(), 1); | ||
assert_eq!(control[0], 42); | ||
let buf = vec![43u8, 0, 0, 0].into_boxed_slice(); | ||
Op::Async(futures::future::ok(buf).boxed()) | ||
Op::Async(futures::future::ok(buf).boxed(), true) | ||
} | ||
Mode::AsyncOptional => { | ||
assert_eq!(control.len(), 1); | ||
assert_eq!(control[0], 42); | ||
let fut = async { | ||
// This future never finish. | ||
futures::future::pending::<()>().await; | ||
let buf = vec![43u8, 0, 0, 0].into_boxed_slice(); | ||
Ok(buf) | ||
}; | ||
Op::Async(fut.boxed(), false) | ||
} | ||
Mode::OverflowReqSync => { | ||
assert_eq!(control.len(), 100 * 1024 * 1024); | ||
|
@@ -853,7 +867,7 @@ pub mod tests { | |
Mode::OverflowReqAsync => { | ||
assert_eq!(control.len(), 100 * 1024 * 1024); | ||
let buf = vec![43u8, 0, 0, 0].into_boxed_slice(); | ||
Op::Async(futures::future::ok(buf).boxed()) | ||
Op::Async(futures::future::ok(buf).boxed(), true) | ||
} | ||
Mode::OverflowResAsync => { | ||
assert_eq!(control.len(), 1); | ||
|
@@ -862,7 +876,7 @@ pub mod tests { | |
vec.resize(100 * 1024 * 1024, 0); | ||
vec[0] = 4; | ||
let buf = vec.into_boxed_slice(); | ||
Op::Async(futures::future::ok(buf).boxed()) | ||
Op::Async(futures::future::ok(buf).boxed(), true) | ||
} | ||
} | ||
}; | ||
|
@@ -953,6 +967,31 @@ pub mod tests { | |
}); | ||
} | ||
|
||
#[test] | ||
fn test_poll_async_optional_ops() { | ||
run_in_task(|cx| { | ||
let (mut isolate, dispatch_count) = setup(Mode::AsyncOptional); | ||
js_check(isolate.execute( | ||
"check1.js", | ||
r#" | ||
Deno.core.setAsyncHandler(1, (buf) => { | ||
// This handler will never be called | ||
assert(false); | ||
}); | ||
let control = new Uint8Array([42]); | ||
Deno.core.send(1, control); | ||
"#, | ||
)); | ||
assert_eq!(dispatch_count.load(Ordering::Relaxed), 1); | ||
// The above op never finish, but isolate can finish | ||
// because the op is an optional async op (blocks_exit flag == false). | ||
assert!(match isolate.poll_unpin(cx) { | ||
Poll::Ready(Ok(_)) => true, | ||
_ => false, | ||
}); | ||
}) | ||
} | ||
|
||
#[test] | ||
fn terminate_execution() { | ||
let (tx, rx) = std::sync::mpsc::channel::<bool>(); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd recommend using
Op::Async
andOp::AsyncOptional
.Op::AsyncOptional
should be kept inIsolate.pending_optional_ops
- which is the same type asIsolate.pending_ops
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I created another version of this at #3721, which uses
AsyncOptional
instead of adding flag toAsync
. I'd like to avoid havingIsolate.pending_optional_ops
because if we have 2FuturesUnordered
list in isolate, it seems to me that it's difficult to poll these 2 futures list correctly.