-
Notifications
You must be signed in to change notification settings - Fork 3.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
Chapter 20: build a web server #584
Conversation
This is based on @Kimundi's scoped_threadpool crate, which is excellent. There are some small differences since we don't use scoped threads.
Here's the draft. The only thing I didn't write was some kind of concluding statement; I'm assuming we'll add that after. |
``` | ||
|
||
A `TcpListener` allows us to listen for TCP connections. We've chosen to listen | ||
to the address `127.0.0.1:8008`. The first four digits are an IP address |
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.
Typo: 8008
should be 8080
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 would love to see TLS included as part of this chapter as well.
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.
Not doable, as that certainly requires an extra crate. :(
@@ -1 +1,1732 @@ | |||
# Un-named project |
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 left this the same for now, as we're gonna have to split it up anyway and figured i'd modify as few files as possible
|
||
This error is fairly cryptic. And that's because the problem is fairly cryptic. Basically, | ||
in order to call a boxed `FnOnce`, it needs to be able to move itself out of the box. But | ||
the compiler doesn't understand that this is okay to do. |
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.
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.
Uhhh... This is quite unfortunate. As a new rustacean, I would probably feel like:
great! now I read the book and understand the language and felt that I can just go implement something. But already in the first non-trivial example there are footguns that I would not have been able to figure out on my own.
While as an experienced Rustacean, I know this is untrue, it does not leave a good impression. Is there no way to get around this issue in another fashion?
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.
There is not.
I'm torn, as in, I do like showing that Rust isn't perfect, but I also hear you.
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.
You could get around this by making this a Box<FnMut>
or Box<Fn>
. Obviously that's less general though.
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 like bstrie's suggestion on the issue:
Use it as an example of how Rust is still a work-in-progress, and provide an on-ramp for people who have finished the book to come work on Rust itself. :)
the plan of attack: | ||
|
||
1. `ThreadPool` will hold on to a sending side of a channel. | ||
2. Each `Worker` will hold on to a recieving side. |
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.
recieving -> receiving
2. Each `Worker` will hold on to a recieving side. | ||
3. The `execute` method of `ThreadPool` will then send the closure it wants | ||
to execute down the sending side of the channel. | ||
4. The `Worker` will loop over its recieving side, and when it gets a job, |
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.
recieving -> receiving
create our new channel, and then have the pool hang on to the sending end. | ||
|
||
If you compile this, it will work, but still have warnings. Let's try passing | ||
the recieving end into our workers. This won't compile yet: |
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.
recieving -> receiving
|
||
Here, `Job` is now holding a trait object; specifically, a boxed closure. We | ||
then send that `job` down the sending end of the channel. Sending may fail; | ||
we use `unwrap` to ignore the error. |
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.
Does unwrap
really ignore the error? To ignore it, we could just let _ =
. Unwrapping let's us safely bring down the system if something goes wrong, rather than continuing to try to run jobs and ignoring that they aren't actually running.
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.
You're right that this could be worded better, I mean "in the moment to not care about it", but if it happens, it certainly won't be ignored.
Finally, we use `call_box` instead of invoking the closure directly. | ||
|
||
This is a very sneaky, complicated trick. Don't worry too much if it doesn't make perfect | ||
sense; someday, it will be completely un-needed. |
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.
un-needed -> unnecessary?
|
||
We need to adjust the `ThreadPool` to send `Message`s rather than `Job`s. | ||
|
||
``` |
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.
``` -> ```rust,ignore
not sure what the ignore does
Inside of our `Worker, instead of receiving a `Job`, we get a `Message`. We then | ||
execute the job if it's a `NewJob`, and break out of our `loop` if it's `Terminate`. | ||
|
||
``` |
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.
``` -> ```rust,ignore
It prevents the example from being run as a test :)
…On Fri, Mar 31, 2017 at 3:38 PM, Josh Leverette ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In second-edition/src/ch20-00-unnamed-project.md
<#584 (comment)>:
> +}
+```
+
+First, we have a new `Message` enum. We have two kinds of messages: "here's a new
+`Job`" and "please terminate execution."
+
+```rust,ignore
+struct ThreadPool {
+ threads: Vec<Worker>,
+ sender: mpsc::Sender<Message>,
+}
+```
+
+We need to adjust the `ThreadPool` to send `Message`s rather than `Job`s.
+
+```
->rust,ignore
not sure what the ignore does
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#584 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AABsiiUTn8LfFqY1qCeiXBTQR3xtMPyLks5rrVYygaJpZM4Mu-3b>
.
|
} | ||
``` | ||
|
||
Inside of our `Worker, instead of receiving a `Job`, we get a `Message`. We then |
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.
`Worker -> `Worker`, i.e. a missing closing `
|
||
There is still more we could do here; for example, our `ThreadPool` is not | ||
inherently tied to HTTP handling, so we could extract it into its own submodule, | ||
or maybe even its own crate! |
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 expect here a sentence alla: Of course there are already very good http server crates and also production ready thread pool implementations on crates.io, so no need to reinvent the wheel in the first place.
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 did mention at the beginning of the chapter that there are crates we could use for this; i'm going to expand a bit more there. There's nothing wrong with reinventing the wheel in order to learn how wheels work :)
Sorry for the drive by review - I was just curious in the new books last chapter and had some thoughts :). Please ignore what does not seem appropriate. |
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'm not quite done with reading this whole chapter but i need to switch contexts for a bit and i don't want to lose this review
@@ -1 +1,1732 @@ | |||
# Un-named project | |||
|
|||
It's been a long journey, but here we are! It's the end of the book. Parting is |
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.
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 read this book so many times as a kid
``` | ||
|
||
A `TcpListener` allows us to listen for TCP connections. We've chosen to listen | ||
to the address `127.0.0.1:8008`. The first four digits are an IP address |
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 first four digits" i would read as 1, 2, 7, and 0, which I don't think is what you meant.... ok to reword this as "the part before the colon"?
networking, people will often talk about "binding to a port", and so the | ||
function is called `bind`. Finally, it returns a `Result<T, E>`; binding may | ||
fail. For example, if we had tried to connect to port 80 without being an | ||
administrator. Since we're writing a basic client here, we're not going to worry |
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.
another thing that might happen is if there are 2 things listening at the same port, like accidentally starting up two instances of this program, right? might be worth using as an example here since it's entirely possible, wdyt?
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.
basic client
should be basic server
``` | ||
|
||
The `incoming` method on `TcpListener` gives us an iterator that gives us a | ||
sequence of streams, more specifically, `TcpStream`s. This struct represents an |
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're probably going to need definitions of "stream" and "connection" here
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.
done
``` | ||
|
||
Right now, "handling" a stream means `unwrap`ping it to ignore any further | ||
errors, and then printing a message. Let's try this code out! First invoke |
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.
what kind of errors might a stream have?
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.
done. that was really annoying to find; i need to write better docs :(
we use `unwrap` to ignore the error. | ||
|
||
Now that we've got the sending side working, let's write the logic of the worker. | ||
Here's a first attempt, but it won't quite work: |
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 think liz is going to be frustrated by all the non-compiling examples in this chapter, but i'm not sure what to do about it....
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.
yeah it's a big refactor so :/
```rust,ignore | ||
let thread = thread::spawn(move ||{ | ||
loop { | ||
let job = job_receiver.lock().unwrap().recv().unwrap(); |
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.
looks like you've changed the signature of Worker::new
from fn new(id: u32, job: Arc<Mutex<mpsc::Receiver<Job>>>) -> Worker {
to fn new(id: u32, job_receiver: Arc<Mutex<mpsc::Receiver<Job>>>) -> Worker {
by this point, I think this should actually be done when we added the Arc Mutex, then the placeholder body of Worker::new
needs to change at that point too
|
||
// and then change this code | ||
impl Worker { | ||
fn new(id: u32, job: Arc<Mutex<mpsc::Receiver<Job>>>) -> Worker { |
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.
job
should change to job_receiver
in the signature and body here, see comment later
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.
fixed
|
||
Here, we first call `lock` on the `job_receiver` to acquire the mutex, then | ||
`unwrap` to panic on any errors, then `recv` to receive a `Job` from the | ||
channel. A final `unwrap` moves past those errors as well. |
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.
plz expand on the different kinds of errors we could get from lock
and recv
respectively and what they'd mean, so that the reader could choose to handle them more gracefully if they want to enhance this program and if it's appropriate
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.
done
|
||
The call to `recv` blocks; that is, if there's no job yet, it will sit here | ||
until one becomes available. The `Mutex<T>` makes sure that only one Worker | ||
at a time tries to request a job. |
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.
does this mean that the first thread to get the lock holds it until it gets a job? i think so.... i kind of want some print statements in here so that we can see this all happening when it's running
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.
not only that but https://github.com/Kimundi/scoped-threadpool-rs/blob/master/src/lib.rs#L117-L122
I wasn't sure what level of detail was appropriate here.
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.
hnnnngggg ok i think people who have done this sort of thing before might notice that the lock is being held longer than it needs to be, but people who haven't probably won't and it would probably be too much information for them.
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.
wait, if we were seeing that behavior, wouldn't calls to /sleep
block the next thread from picking up any other requests? i'm not seeing that behavior...
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.
where "that behavior" = locking the jobs for the time it takes to run a job
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.
yeah, i am not sure about this. i thought because it was a temporary, this is okay, but i was not actually sure.
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 guess it doesn't matter since we're not gonna mention it anyway :)
|
||
This error is fairly cryptic. And that's because the problem is fairly cryptic. Basically, | ||
in order to call a boxed `FnOnce`, it needs to be able to move itself out of the box. But | ||
the compiler doesn't understand that this is okay to do. |
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 like bstrie's suggestion on the issue:
Use it as an example of how Rust is still a work-in-progress, and provide an on-ramp for people who have finished the book to come work on Rust itself. :)
This is a very sneaky, complicated trick. Don't worry too much if it doesn't make perfect | ||
sense; someday, it will be completely un-needed. | ||
|
||
With this trick, our thread pool is in a working state! Give it a `cargo run`, and make |
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.
Ok are we going with "thread pool" or "threadpool"? You've used both....
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.
thread pool
} | ||
|
||
// we use this instead of (job.job)(); | ||
job.job.call_box(); |
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 line goes in Worker::new
, right? I'll do the // ...snip...
thing and put it in context, just want to be sure
Worker 2 got a job; executing. | ||
``` | ||
|
||
Success! We now have a threadpool executing connections asynchronously. |
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.
wdyt about, either here or somewhere else, looking for a request to GET /long
etc, and if we see that, sleeping for 30s to simulate a long-running request? Then we could make other requests and see that they're not blocked on the long one, and see that the thread that's handling the long request isn't handling the other requests? I just want to be able to see and prove to myself that this is working :)
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.
and we can add that in above to motivate this change
|
||
Because we only have a `&mut` in `drop`, we cannot actually call `join`, as `join` | ||
takes its argument by value. What to do? Well, we already have a way to represent | ||
"something or nothing", and that's `Option<T>`. Let's update the definition of |
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.
wait what i'm lost... how does "we cannot actually call join
, as join
takes its argument by value" then imply that we should use Option
????
reading on, that's not really the logic that i understand here: we need to use Option
because we need to be able to take the thread out of the worker and leave None
in its place, like we did in the OOP chapter with the state
field, right?
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.
yup
|
||
```rust,ignore | ||
struct ThreadPool { | ||
threads: Vec<Worker>, |
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.
Can we change the threads
field on ThreadPool
to Workers
at this point? It started hurting my head later on, especially in the drop implementation, where we were doing for worker in self.threads; worker.thread
and i couldn't keep track of what was what :-/
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.
sure thing (i was switching between them as i wrote this...)
|
||
We need two loops here. Why? Well, if we send a message and then try to join, | ||
it's not guaranteed that that worker will be the one that gets that message. | ||
We'd then deadlock. So we first put all of our `Terminate` messages on the |
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.
Can you talk through the deadlock situation a little bit more?
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.
done
We need two loops here. Why? Well, if we send a message and then try to join, | ||
it's not guaranteed that that worker will be the one that gets that message. | ||
We'd then deadlock. So we first put all of our `Terminate` messages on the | ||
channel, and then we join on all the threads. |
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.
How are we guaranteed to send exactly one Terminate
message to each worker? Because the Terminate
message causes the worker to stop listening for more requests immediately, right? So another worker would have to be the one to pick up the Terminate
message?
Is there a case where a Worker
would have terminated already, before getting a Terminate
message? (we do have unwrap
s everywhere lol) What would happen then, if we send more Terminate
messages than workers? sending the last Terminate
message would panic then, right?
Could we talk about this or why this isn't possible if i'm wrong? :)
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.
How are we guaranteed to send exactly one Terminate message to each worker? Because the Terminate message causes the worker to stop listening for more requests immediately, right? So another worker would have to be the one to pick up the Terminate message?
Yup!
Is there a case where a Worker would have terminated already, before getting a Terminate message? (we do have unwraps everywhere lol)
Yeah 😓
What would happen then, if we send more Terminate messages than workers? sending the last Terminate message would panic then, right?
Sending it wouldn't, but when the pool gets dropped, the join
for that thread would return Err
, and that unwrap would panic. In this case, our program is ending anyway... maybe we should not unwrap
that final join
. Dunno.
/// | ||
/// # Panics | ||
/// | ||
/// The `new` function will panic if the size is zero. |
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.
Oh also, doc comments in main.rs
aren't used, which further helps the case i was going to make that we should have a small main.rs
and be doing most of this code in lib.rs
, i'm starting to feel itchy with everything being in main.rs
|
||
```rust,ignore | ||
struct Job { | ||
job: Box<FnOnce() + Send + 'static>, |
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.
Potentially a bad idea but I want to see what you think: we haven't talked about type aliases at all yet. What if we add a section on type aliases to chapter 19 (which you mentioned felt light, this would make it a tiny bit more meaty), and then reuse that knowledge here to make Job
be a type alias for Box<FnOnce() + Send + 'static>
instead of a struct with a job
field holding that type?
The job.job
business was kind of bothering me, so this idea is somewhat an attempt to eliminate that.
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.
fun trivia fact: the original code I am building this off of does use a type alias here, called Thunk
. I thought it was a little jargon-y and am generally ambivalent about aliases. But yeah, this could work.
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.
boooo i dont think the type alias is going to work (or work as nicely as i was thinking) because of the FnBox trick we have to do
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.
WAIT NEVERMIND I GOT IT TO WORK I UNDERSTAND EVERYTHING NOW
Ok, done now :) I'm happy to take care of any of these things, but if you've got time to help next week and want to take care of them, that would be awesome. I'll pick up whatever you haven't taken care of late next week, unless you register disagreement by then :) |
function is called `bind`. Finally, it returns a `Result<T, E>`; binding may | ||
fail. For example, if we had tried to connect to port 80 without being an | ||
administrator. Since we're writing a basic client here, we're not going to worry | ||
about handling these kinds of errors, and so `unwrap` lets us ignore them. |
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.
no ?
in main :(
} | ||
``` | ||
|
||
Here's the idea: we loop through each of our `threads`, using `&mut` because `self` is |
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.
Isn't iter_mut
more idiomatic ?
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 don't believe so, i'm open to either
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 so nice that this book has project chapters to show how a more complete rust project looks. I haven't gone through this chapter as closely as I have on some of the others, but these are the things I noticed trying to 'follow along at home', and some thoughts on the ending.
I guess this is also a good time to say thank you so much for spending all the time writing, editing, rewriting and taking feedback for this book! It's been great seeing this book develop. 👍 ❤️
|
||
let get = b"GET / HTTP/1.1\r\n"; | ||
|
||
let start = &buffer[..get.len()]; |
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 seems to be reinventing starts_with
?
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.
nice catch 😅
} | ||
|
||
ThreadPool { | ||
threads: threads, |
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 guess the shorthand for this is being avoided for now.
Edit: and there are places in the chapter where the shorthand goes from being usable to no being usable, so maybe it's not really worth it here.
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.
only because it wasn't stable when i wrote this ;using the shorthand is 👍
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.
Edit: and there are places in the chapter where the shorthand goes from being usable to no being usable, so maybe it's not really worth it here.
Actually I decided to change the names of variables so that the shorthand is usable in all but one place-- I really like this change because we changed the type being sent over the channel from Job
to Message
, so job_sender
/job_receiver
weren't even what they said they were by the end :) Now they're just sender
/receiver
though!
|
||
impl Worker { | ||
fn new(id: usize) -> Worker { | ||
let thread = thread::spawn(|| { }); |
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.
Space inside { }
?
pub fn new(size: usize) -> ThreadPool { | ||
assert!(size > 0); | ||
|
||
let (job_sender, job_receiver) = mpsc::channel::<Job>(); |
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.
Explicit type for clarity? It seems it can be inferred.
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 thought it couldn't but I might be wrong
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 can be inferred! Neat, I guess because the ends get passed into functions that have the types.
impl Worker { | ||
fn new(id: usize, job_receiver: Arc<Mutex<mpsc::Receiver<Message>>>) -> | ||
Worker { | ||
// ...snip... |
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 don't think this code should have been snipped: it's changed to involve a match.
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.
Yup I messed up!
println!("Worker {} got a job; executing.", id); | ||
|
||
job.call_box(); | ||
}, |
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.
Commas after }
in match blocks?
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.
That's what I prefer. Has the style team ruled on this for rustfmt yet? We should probably just run rustfmt on everything right before publishing....
- Add more documentation to `ThreadPool` and its public methods | ||
- Add tests of the library's functionality | ||
- Change calls to `unwrap` to more robust error handling | ||
- Use `ThreadPool` to perform some other task rather than serving web requests |
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.
Try using a library from crates.io to create a similar web server? 😄
Well done! You've made it to the end of the book! We'd like to thank you for | ||
joining us on this tour of Rust. You're now ready to go out and implement your | ||
own Rust projects. Remember there's a community of other Rustaceans who would | ||
love to help you with any challenges you encounter on your Rust journey. |
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.
Maybe mention other resources? Most of the obvious ones get mentioned somewhere in the book though.
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.
Yeah, i'll think about this when we revise chapter 1.
|
||
Well done! You've made it to the end of the book! We'd like to thank you for | ||
joining us on this tour of Rust. You're now ready to go out and implement your | ||
own Rust projects. Remember there's a community of other Rustaceans who would |
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.
Or help with other people's?
I want to heart this but gh still hasn't implemented reactions on review summary comments :P Thank you so much for all your excellent feedback!!!! Are you coming to RustConf or Rust Belt Rust this year by chance? I'd love to buy you a {beverage/dessert/something of choice} in thanks!!! |
I think this is ready for another review @steveklabnik @matthewjasper anyone else? |
|
||
Then create a new directory, *src/bin*, and move the binary crate rooted in | ||
*src/main.rs* into *src/bin/main.rs*. This will make the library crate be the | ||
primary crate in the *hello* directory; we can still run the binary in |
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 still probably keep it src/main.rs
personally. Why'd you use the secondary binary location?
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.
$ mv src/bin/main.rs src/main.rs
$ cargo doc --open
error: cannot document a package where a library and a binary have the same name. Consider renaming one or marking the target as `doc = false`
We demonstrate doc comments, so I want to be able to build docs. It's either move main into bin or fuss with Cargo.toml, and I went with moving the file :-/
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.
ah ha!
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.
sooo is that a orrrrr....?
… On Tue, May 16, 2017 at 10:35 AM, Carol (Nichols || Goulding) < ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In second-edition/src/ch20-03-designing-the-interface.md
<#584 (comment)>:
> +that we're doing in our web server. Once we've got the thread pool library
+written, we could use that functionality to do whatever work we want to do, not
+just serve web requests.
+
+So create *src/lib.rs* that contains the simplest definition of a `ThreadPool`
+struct that we can have for now:
+
+<span class="filename">Filename: src/lib.rs</span>
+
+```rust
+pub struct ThreadPool;
+```
+
+Then create a new directory, *src/bin*, and move the binary crate rooted in
+*src/main.rs* into *src/bin/main.rs*. This will make the library crate be the
+primary crate in the *hello* directory; we can still run the binary in
sooo is that a [image: ] orrrrr....?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#584 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AABsipadDMQK_WT8Z-lKZUJd5N0fcoUbks5r6bRFgaJpZM4Mu-3b>
.
|
:confetti-ball:
|
The first half is here; second half coming tomorrow.