-
-
Notifications
You must be signed in to change notification settings - Fork 582
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
WIP - Async commands #144
base: master
Are you sure you want to change the base?
WIP - Async commands #144
Conversation
… is async then the result of command will be ChannelResultAsync, otherwise ChannelResultSync
|
||
// Handle response arguments to issued command | ||
fn execute_message<M: ChannelMessageMode>(&mut self) { | ||
let channel_result = M::handle(Box::leak(self.message.clone().into_boxed_str())); |
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 is unsafe, It should be avoided
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.
It's just workaround for testing...
{ | ||
self.thread_pool.lock().unwrap().scope(|s| { | ||
s.spawn(move |_| func()); | ||
}); |
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.
scope
is executing tasks within scoped thread pool, the thing is that this operation blocks the main thread. I need to rework that.
@valeriansaliou I've done some things and almost succeed. There is one thing that is still blocking asynchronous execution, but I have already plan how to solve it. Could you make a code review? |
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.
The build is not passing, because of feature(fnbox) - use FnMut 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.
Still no unit/integration tests...
Thanks a ton!! I’ll be reviewing this soon, had no chance so far as my week is unusually busy with Crisp. |
I could give your changes a quick look, and they look good to me. I'd say you can continue on this path! |
Great, I will! |
Keep me posted on your progress on this one, I have a bit more time for reviews :) |
Fine, I need to find some time |
There are conflicts to resolve, and probably some changes might be required due to the latest Rust async improvements. |
ref: #43
In progres...