Handle duplicated requests #54
Conversation
|
CI failure seems like our random_port hit a restricted port 🤔. Maybe use predefined list of ports for tests in the future? |
I think a good start would be defining a test in We can take advantage of the fact that the code is designed to separate the accepting of the request from actually handling the request. I.e., The test could spawn the server, move it to another thread and have it start listening with The test could then open a client socket and send two Rrqs in a row. The test should assert that It's important to not call the
Nah. It's a rare enough occurrence that I can just retrigger it. If it starts becoming more of a problem I might just serialize the tests. |
|
The tests implementations are a bit messy, with pieces copied from the actual client.rs edited to behave incorrectly. Or we could use proper code from the client module for all the normal behavior tests (which is 3 of the 4 I've written), and use the hand-rolled implementation for the duplicate request test alone. What do you think? |
|
You could look at my |
src/server.rs
Outdated
| @@ -78,7 +94,18 @@ impl Server { | |||
| let addr = self.socket.local_addr()?.ip().to_string(); | |||
| let bind_to = format!("{}:{}", addr, port); | |||
|
|
|||
| Handler::new(bind_to, src_addr, direction, self.serve_dir.clone()) | |||
| let active_clients = Arc::clone(&self.active_clients); | |||
| let callback: Callback = Some(Box::new(move || { | |||
There was a problem hiding this comment.
Couldn't we avoid the Callback allocation + indirection by having a separate struct that holds the active_clients Arc and does the same thing as this callback? It's not like we ever expose Handler to the outside world and thus would ever have multiple callbacks. :>
src/server.rs
Outdated
| }); | ||
| match server.serve() { | ||
| Err(e) => assert_eq!(e.kind(), io::ErrorKind::AddrNotAvailable), | ||
| _ => (), |
There was a problem hiding this comment.
The test should fail if server.serve() does not return an error after the first request. You can do this by just panic!-ing in this branch.
src/server.rs
Outdated
| pub fn handle(mut self) -> Result<()> { | ||
| let callback = match self.callback.take() { | ||
| Some(l) => l, | ||
| None => Box::new(|| {}), |
There was a problem hiding this comment.
Is there a case in normal operation where this would be None? Our Handler is moved into Handler::handle, therefore we won't ever call it again.
| None => Box::new(|| {}), | |
| None => unreachable!(), |
src/server.rs
Outdated
|
|
||
| assert_eq!(&exemplar[..], &res[..]); | ||
| } | ||
| { |
There was a problem hiding this comment.
What does this block accomplish? We've already finished the previous request and dropped the socket by this point, so (as far as I understand the code) the src_addr would already be out of active_clients, right?
src/server.rs
Outdated
| h.handle().unwrap(); | ||
| }))); | ||
| } | ||
| for i in 0..n { |
There was a problem hiding this comment.
You can avoid having to have a Vec<Option<...>> by choosing to instead iterate over the Vec directly (instead of using indices), as that will move your elements out of the vec progressively
|
You could also just move all the |
But in that case, if the Handler error out before getting to the end, the callback would not be called and the TID would never be released. |
|
You can keep the logic inline in the |
|
I agree with @PurpleMyst. Let's drop the callback. Any structs that need access can just have a ClientsPool reference (Arc<Mutex>). It can be kept in the |
Fixes #49.
I tried editing the client.rs to send duplicated request. The server errored as expected.
But beyond that, I'm at lost on how to test this, or write automated tests for it.
I'm still a beginner in Rust, so if I'm doing anything wrong or unidiomatic, please do tell!