From 786047629a843d2ea5328096986f1bb375c1ec96 Mon Sep 17 00:00:00 2001
From: Jacob Hoffman-Andrews <github@hoffman-andrews.com>
Date: Sat, 26 Sep 2020 20:05:03 -0700
Subject: [PATCH] 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.
---
 src/pool.rs            |  6 ------
 src/test/agent_test.rs | 48 +++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 47 insertions(+), 7 deletions(-)

diff --git a/src/pool.rs b/src/pool.rs
index 8d66d40..14b0da7 100644
--- a/src/pool.rs
+++ b/src/pool.rs
@@ -415,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();
-    }
-}
diff --git a/src/test/agent_test.rs b/src/test/agent_test.rs
index ea107ec..d81bc4d 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();
+    assert!(resp.ok(), format!("error: {}", resp.status()));
+    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();
+    assert!(
+        resp_to_be_dropped.ok(),
+        format!("error: {}", resp_to_be_dropped.status())
+    );
+    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();
+    assert!(
+        resp_to_succeed.ok(),
+        format!("error: {}", resp_to_succeed.status())
+    );
+
+    Ok(())
+}