-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
feat: add spawn and try_current for madsim-tokio #197
Conversation
Signed-off-by: Kevin Axel <kevinaxel@163.com>
Signed-off-by: Kevin Axel <kevinaxel@163.com>
Signed-off-by: Kevin Axel <kevinaxel@163.com>
Signed-off-by: Kevin Axel <kevinaxel@163.com>
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.
Rest LGTM! Thanks
pub fn try_read_buf<B: BufMut>(&mut self, buf: &mut B) -> io::Result<usize> { | ||
unimplemented!(); | ||
} |
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.
Will this be implemented in the future? If so, we can use todo!()
.
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.
This just used to pass the sqlx's compile, so maybe we can just leave a unimplemented here? So far sqlx hasn't called this on TcpStream
yet.
F::Output: Send + 'static, | ||
{ | ||
self.create_node() | ||
.name("spawn a task") |
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.
We shouldn't create a new node here. The task should be spawned within the current node. Please use Spawner::current()
instead.
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 have tried Spawner::current
before, but in some cases, like dropping some Future
and calling a sync drop
to spawn an async task which means there are no tasks in the runtime. The drop site is out the scope of
madsim/madsim/src/sim/task/mod.rs
Line 270 in ca93352
let _guard = crate::context::enter_task(info.clone()); |
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 see. In this case, I think we should enter task context before dropping the task. And here we should still use Spawner::current()
.
to support madsim-rs/sqlx, we add some
spawn
,try_current
and some other dummy interface.