From 50d9ccff8ccc167a49faba7ae1585db4bcdf114d Mon Sep 17 00:00:00 2001
From: Jacob Hoffman-Andrews <github@hoffman-andrews.com>
Date: Sun, 18 Apr 2021 10:46:20 -0700
Subject: [PATCH] Don't reuse conns with bytes pending from server (#372)

This makes us less likely to try and reuse a closed connection, which
produces problems in particular for requests that can't be retried.

Fixes #361
Fixes #124
---
 src/stream.rs          | 19 ++++++++++++++++---
 src/test/agent_test.rs | 40 ++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+), 3 deletions(-)

diff --git a/src/stream.rs b/src/stream.rs
index 5ed48ca..7773fba 100644
--- a/src/stream.rs
+++ b/src/stream.rs
@@ -148,7 +148,15 @@ impl Stream {
 
     // Check if the server has closed a stream by performing a one-byte
     // non-blocking read. If this returns EOF, the server has closed the
-    // connection: return true. If this returns WouldBlock (aka EAGAIN),
+    // connection: return true. If this returns a successful read, there are
+    // some bytes on the connection even though there was no inflight request.
+    // For plain HTTP streams, that might mean an HTTP 408 was pushed; it
+    // could also mean a buggy server that sent more bytes than a response's
+    // Content-Length. For HTTPS streams, that might mean a close_notify alert,
+    // which is the proper way to shut down an idle stream.
+    // Either way, bytes available on the stream before we've made a request
+    // means the stream is not usable, so we should discard it.
+    // If this returns WouldBlock (aka EAGAIN),
     // that means the connection is still open: return false. Otherwise
     // return an error.
     fn serverclosed_stream(stream: &std::net::TcpStream) -> io::Result<bool> {
@@ -156,8 +164,13 @@ impl Stream {
         stream.set_nonblocking(true)?;
 
         let result = match stream.peek(&mut buf) {
-            Ok(0) => Ok(true),
-            Ok(_) => Ok(false), // TODO: Maybe this should produce an "unexpected response" error
+            Ok(n) => {
+                debug!(
+                    "peek on reused connection returned {}, not WouldBlock; discarding",
+                    n
+                );
+                Ok(true)
+            }
             Err(e) if e.kind() == io::ErrorKind::WouldBlock => Ok(false),
             Err(e) => Err(e),
         };
diff --git a/src/test/agent_test.rs b/src/test/agent_test.rs
index 36edeaa..b7d254c 100644
--- a/src/test/agent_test.rs
+++ b/src/test/agent_test.rs
@@ -4,6 +4,7 @@ use crate::error::Error;
 use crate::testserver::{read_request, TestServer};
 use std::io::{self, Read, Write};
 use std::net::TcpStream;
+use std::thread;
 use std::time::Duration;
 
 use super::super::*;
@@ -17,6 +18,18 @@ fn idle_timeout_handler(mut stream: TcpStream) -> io::Result<()> {
     Ok(())
 }
 
+// Handler that answers with a simple HTTP response, and times
+// out idle connections after 2 seconds, sending an HTTP 408 response
+fn idle_timeout_handler_408(mut stream: TcpStream) -> io::Result<()> {
+    read_request(&stream);
+    stream.write_all(b"HTTP/1.1 200 OK\r\nContent-Length: 8\r\n\r\nresponse")?;
+    let twosec = Duration::from_secs(2);
+    stream.set_read_timeout(Some(twosec))?;
+    thread::sleep(twosec);
+    stream.write_all(b"HTTP/1.1 408 Request Timeout\r\nContent-Length: 7\r\n\r\ntimeout")?;
+    Ok(())
+}
+
 #[test]
 fn connection_reuse() {
     let testserver = TestServer::new(idle_timeout_handler);
@@ -44,6 +57,33 @@ fn connection_reuse() {
     assert_eq!(resp.status(), 200);
 }
 
+#[test]
+fn connection_reuse_with_408() {
+    let testserver = TestServer::new(idle_timeout_handler_408);
+    let url = format!("http://localhost:{}", testserver.port);
+    let agent = Agent::new();
+    let resp = agent.get(&url).call().unwrap();
+
+    // use up the connection so it gets returned to the pool
+    assert_eq!(resp.status(), 200);
+    resp.into_string().unwrap();
+
+    assert!(agent.state.pool.len() > 0);
+
+    // wait for the server to close the connection.
+    std::thread::sleep(Duration::from_secs(3));
+
+    // try and make a new request on the pool. this fails
+    // when we discover that the TLS connection is dead
+    // first when attempting to read from it.
+    // Note: This test assumes the second  .call() actually
+    // pulls from the pool. If for some reason the timed-out
+    // connection wasn't in the pool, we won't be testing what
+    // we thought we were testing.
+    let resp = agent.post(&url).send_string("hello".into()).unwrap();
+    assert_eq!(resp.status(), 200);
+}
+
 #[test]
 fn custom_resolver() {
     use std::io::Read;