Add try_recv_timeout function to the server

[moises-silva]

Please be warned I'm new to rust :)

This allows giving up on receiving a request after a specified timeout duration.

It's working for me.

I wouldn't strictly need this if I knew how to kill/stop the thread pool from another thread and force the recv() to unblock. In C you can kill the thread and if blocked on a syscall you get an error back from the syscall, you can check EINTR and decide whether to quit or issue the syscall again. I'm not sure how one would implement such a thing in rust, so the next best thing was to simply timeout on recv() and I can check out on my app once in a while for a shutdown flag.

[tomaka]

Thanks for the PR!

Two remarks:

• The functions should probably be named recv_timeout() and pop_timeout() instead, as the try prefix means "don't block".
• You should put a loop around pop_timeout(), like pop() does, because it is possible for the condvar to wake up for no reason.

Something like this:

    pub fn try_pop_timeout(&self, timeout: Duration) -> Option<T> {
        let mut queue = self.queue.lock().unwrap();
        loop {
          if let Some(elem) = queue.pop_front() {
              return Some(elem);
          }
          let result;
          (queue, result) = self.condvar.wait_timeout(queue, timeout).unwrap();
          if result.timed_out() {
              return None;
          }
       }
    }

[moises-silva]

@tomaka Thanks for the feedback.

I did consider that naming before but went with try because it may or may not succeed in receiving a request. I'll rename them as per your advice.

I happened to have considered the second remark as well :) , however going back to sleep if we were waked up for no reason can result in sleeping for significantly more time than specified unless I keep track of how much time I slept on the previous iteration(s). For my use case (and actually in general) I think is preferable to be waken up sooner and the application code can go and check for other conditions (e.g shutdown) and then go back to recv timeout. As long as we document the weak guarantee (e.g that will sleep at most x ms but could be less).

Let me know if you agree or not.

[tomaka]

Usually when a function has a timeout (in Rust or in general), it means that this function will wait for at least the given amount, but can wait more. On Windows for example, thread::sleep can round up for up to several milliseconds. I don't think checking a variable will take a significant time.

[moises-silva]

I'd be fine with a few milliseconds, but if I specify 1000ms and the function returns until 1900ms, that's pretty bad. Say the condition returns due to 'no reason' at 900ms, if I go check the queue, there's nothing and go back to sleep, now I may end up sleeping 1900ms in total.

If you feel strongly to get this change to get this merged I'll make the change and track the slept time to reduce the possible extra delay.

[tomaka]

Oh I see what you mean. In fact yeah, you should decrease the remaining timeout.
In fact a cleaner way would probably be to calculate the time when the function should stop, and do something like expected_end - now when calling the wait function.

[frewsxcv]

What's the point of the loop if all branches of the match end up returning?

[tomaka]

I think that's a mistake in the existing code. The other functions have this unnecessary loop as well, and the new one was probably copy-pasted from the others.

[moises-silva]

yup, sorry about that, I should've reviewed the code I copied better.

[moises-silva]

Yeah when I copied the code I missed that the loop is actually not needed. I've done a new commit addressing your comments and removing the loop from the other functions as well.

@tomaka note that I did not do the 'expected end' calculation because in any case I need to know the remaining sleep time for the next iteration to pass a smaller duration to the condvar.wait_timeout function, so if I'm already calculating the remaining sleep time there's no need to also calculate the expected end time.

Let me know if you have any further comments.

[frewsxcv]

Looks great, thanks!