Repository: algesten/ureq
Pull request #160 (2020-09-27 03:09:52)

bugfix: Don't re-pool streams on drop.
We only want streams in the pool that are ready for the next request,
and don't have any remaining bytes of the previous request. That's the
case when we return a stream due to EOF on the underlying `Read`.

However, there was also an `impl Drop` that returned the stream to the
pool on drop of a PoolReturnRead. That resulted in a bug: If a user
called `response.into_reader()`, but did not read the entire body,
a dirty stream would be returned to the pool. The next time a request
was made to the same host, that stream would be reused, and instead of
reading off "HTTP/1.1 200 OK", ureq would read a remnant of the previous
request, resulting in a BadStatus error.


Patch:
diff --git a/src/pool.rs b/src/pool.rs
index 5c3482f..14b0da7 100644
--- a/src/pool.rs
+++ b/src/pool.rs
@@ -79,13 +79,6 @@ impl ConnectionPool {
             return;
         }
 
-        if max_connections == 0 {
-            // Clear the connection pool, caching is disabled.
-            self.lru.clear();
-            self.recycle.clear();
-            return;
-        }
-
         // Remove any extra connections if the number was decreased.
         while self.lru.len() > max_connections {
             self.remove_oldest();
@@ -422,9 +415,3 @@ impl<R: Read + Sized + Into<Stream>> Read for PoolReturnRead<R> {
         Ok(amount)
     }
 }
-
-impl<R: Read + Sized + Into<Stream>> Drop for PoolReturnRead<R> {
-    fn drop(&mut self) {
-        self.return_connection();
-    }
-}


Test patch:
diff --git a/src/test/agent_test.rs b/src/test/agent_test.rs
index ea107ec..97568b3 100644
--- a/src/test/agent_test.rs
+++ b/src/test/agent_test.rs
@@ -2,7 +2,7 @@
 
 use crate::test;
 use crate::test::testserver::{read_headers, TestServer};
-use std::io::{self, Write};
+use std::io::{self, Read, Write};
 use std::net::TcpStream;
 use std::time::Duration;
 
@@ -183,3 +183,49 @@ fn test_cookies_on_redirect() {
     assert!(agent.cookie("second").is_some());
     assert!(agent.cookie("third").is_some());
 }
+
+#[test]
+fn dirty_streams_not_returned() -> io::Result<()> {
+    let testserver = TestServer::new(|mut stream: TcpStream| -> io::Result<()> {
+        read_headers(&stream);
+        stream.write_all(b"HTTP/1.1 200 OK\r\n")?;
+        stream.write_all(b"Transfer-Encoding: chunked\r\n")?;
+        stream.write_all(b"\r\n")?;
+        stream.write_all(b"5\r\n")?;
+        stream.write_all(b"corgi\r\n")?;
+        stream.write_all(b"8\r\n")?;
+        stream.write_all(b"dachsund\r\n")?;
+        stream.write_all(b"0\r\n")?;
+        stream.write_all(b"\r\n")?;
+        Ok(())
+    });
+    let url = format!("http://localhost:{}/", testserver.port);
+    let agent = Agent::default().build();
+    let resp = agent.get(&url).call();
+    if let Some(err) = resp.synthetic_error() {
+        panic!("resp failed: {:?}", err);
+    }
+    let resp_str = resp.into_string()?;
+    assert_eq!(resp_str, "corgidachsund");
+
+    // Now fetch it again, but only read part of the body.
+    let resp_to_be_dropped = agent.get(&url).call();
+    if let Some(err) = resp_to_be_dropped.synthetic_error() {
+        panic!("resp_to_be_dropped failed: {:?}", err);
+    }
+    let mut reader = resp_to_be_dropped.into_reader();
+
+    // Read 9 bytes of the response and then drop the reader.
+    let mut buf = [0_u8; 4];
+    let n = reader.read(&mut buf)?;
+    assert_ne!(n, 0, "early EOF");
+    assert_eq!(&buf, b"corg");
+    drop(reader);
+
+    let resp_to_succeed = agent.get(&url).call();
+    if let Some(err) = resp_to_succeed.synthetic_error() {
+        panic!("resp_to_succeed failed: {:?}", err);
+    }
+
+    Ok(())
+}