Barbara builds an async executor #115
Conversation
tmandry
left a comment
tmandry
left a comment
There was a problem hiding this comment.
Nicely written! I was able to follow along quite well and each step along the way made sense. I added a few comments that might help improve the story somewhat.
| } | ||
| ``` | ||
|
|
||
| "How to create a waker?" Barbara asked herself. Quickly, she found the `Wake` trait. After reading the documentation, she realized the task in the scheduler should be stored in an `Arc`. And to implement `Wake`, the `Task` should also contain the sender of the scheduler. She changed the code to something like this: |
There was a problem hiding this comment.
At this point I'm thinking about building an I/O reactor, so it might help to explain (here or above) that this is for compute tasks, not async I/O ones.
There was a problem hiding this comment.
It'd be helpful I think to say why the task should be in an arc. Presumably the idea is that the Waker will need to hold a reference to the task so that it can be scheduled; I guess it will need to also hold a reference to the queue or something like that?
There was a problem hiding this comment.
Oh, I guess you said that last part ('the sender of the scheduler")
There was a problem hiding this comment.
Oh, and I guess the Arc is partly because of the waker API...
|
|
||
| Luckily, a colleague of Barbara noticed something wrong. The `wake` method could be called multiple times so multiple copies of the task could exist in the scheduler. The scheduler might not work correctly because of it and a more severe problem was that multiple threads might get copies of the same task from the scheduler and cause a race in polling the future. | ||
|
|
||
| Barbara soon got a idea to solve it. She added a state field to the `Task`. By carefully maintaining the state of the task, she could guarantee there are no duplicate tasks in the scheduler and no race can happen when polling the future: |
There was a problem hiding this comment.
Just as a suggestion, maybe this paragraph can say something about how she spends a lot of effort in writing the code below (at least, it would take me a lot of effort). Then the reader is already focused on the pain of having to write this to satisfy such a simple requirement.
|
|
||
| *Here are some standard FAQ to get you started. Feel free to add more!* | ||
|
|
||
| * **What are the morals of the story?** |
| } | ||
| ``` | ||
|
|
||
| "How to create a waker?" Barbara asked herself. Quickly, she found the `Wake` trait. After reading the documentation, she realized the task in the scheduler should be stored in an `Arc`. And to implement `Wake`, the `Task` should also contain the sender of the scheduler. She changed the code to something like this: |
There was a problem hiding this comment.
It'd be helpful I think to say why the task should be in an arc. Presumably the idea is that the Waker will need to hold a reference to the task so that it can be scheduled; I guess it will need to also hold a reference to the queue or something like that?
| } | ||
| ``` | ||
|
|
||
| "How to create a waker?" Barbara asked herself. Quickly, she found the `Wake` trait. After reading the documentation, she realized the task in the scheduler should be stored in an `Arc`. And to implement `Wake`, the `Task` should also contain the sender of the scheduler. She changed the code to something like this: |
There was a problem hiding this comment.
Oh, I guess you said that last part ('the sender of the scheduler")
| } | ||
| ``` | ||
|
|
||
| "How to create a waker?" Barbara asked herself. Quickly, she found the `Wake` trait. After reading the documentation, she realized the task in the scheduler should be stored in an `Arc`. And to implement `Wake`, the `Task` should also contain the sender of the scheduler. She changed the code to something like this: |
There was a problem hiding this comment.
Oh, and I guess the Arc is partly because of the waker API...
|
This story is really good. I left various comments, but I don't think it needs much. |
nikomatsakis
left a comment
nikomatsakis
left a comment
There was a problem hiding this comment.
(Marking as Changes Requested)
sticnarf
left a comment
sticnarf
left a comment
There was a problem hiding this comment.
Apply some suggestions.
Yes, the API is the biggest hint. And it can take some while to think out why it needs an |
rylev
left a comment
rylev
left a comment
There was a problem hiding this comment.
Awesome! I left a few small suggestions, but it is already looking great.
Co-authored-by: Ryan Levick <[email protected]>
nikomatsakis
left a comment
nikomatsakis
left a comment
There was a problem hiding this comment.
one more nit, per @rylev
|
I'm going to go ahead and merge this! |
5 participants
A "status quo" story about my first experience about writing an async executor.