Only one connection being handled at a time

[gmbeard]

I believe that the placement of the call to pool.scoped inside the loop statement isn't taking advantage of the thread pool. As it stands, I think only one connection at a time is being processed. The example in the scoped_threadpool documentation suggests that the entire loop be enclosed in the Pool::scoped closure.

https://github.com/steveklabnik/simple-server/blob/10103f59775591a470dc4d0275cfe54e635e9a5c/src/lib.rs#L254-L269

[gmbeard]

I think this should be an easy fix; Just rearrange the call to pool.scoped(|scope|{ so that it encapsulates the loop { ... }. This is just an observation. I haven't done any testing to prove my theory.

[scurest]

I can confirm this. I tested it with this server

extern crate simple_server;

fn main() {
    use simple_server::Server;
    let server = Server::new(|request, mut response| {
        if request.uri() == "/sleep" {
            std::thread::sleep(std::time::Duration::from_secs(5));
            Ok(response.body(b"Yawn\n".to_vec())?)
        } else {
            Ok(response.body(b"I'm ready!\n".to_vec())?)
        }
    });
    server.listen("127.0.0.1", "7878");
}

If the server uses one thread, if you request /sleep and then /, the / request should block until the /sleep one finishes. If it uses multiple threads, the / request should return without waiting for the /sleep one. Currently it blocks. Rearranging pool.scoped(|scope|{ and the loop fixes it.

This also seems to fix the bug where if the handler panics the whole server goes down :+1:

[scriptjunkie]

I have tested the following, and can confirm it works:

    pub fn listen_on_socket(&self, listener: TcpListener) -> ! {
        const READ_TIMEOUT_MS: u64 = 20;
        let num_threads = self.pool_size();
        let mut pool = Pool::new(num_threads);
        let mut incoming = listener.incoming();

        pool.scoped(|scope| {
			loop {
				// Incoming is an endless iterator, so it's okay to unwrap on it.
				let stream = incoming.next().unwrap();
				let stream = stream.expect("Error handling TCP stream.");

				stream
                .set_read_timeout(Some(Duration::from_millis(READ_TIMEOUT_MS)))
                .expect("FATAL: Couldn't set read timeout on socket");

                scope.execute(|| {
                    self.handle_connection(stream).map_err(|e| match e{
							Error::Io(err) => println!("Error handling connection {}", err),
							_ => {}
						}
					);
                });
			}
        })
    }

this also prevents a handler panic from bringing a thread down. (otherwise, it might seem like it continues to work, but I think you have still killed one of the threads in the thread pool)