Repository: algesten/ureq
Pull request #197 (2020-10-21 07:28:25)

Check error immediately after Response::from_read
This is a fix for the issue reported in the second comment at https://github.com/algesten/ureq/issues/196#issuecomment-712863514. Note that it's a draft because this breaks at least one existing test: `overall_timeout_during_headers`. That's because previously, the overall timeout was getting preserved on the stream, and `into_string()` was returning an I/O error. With this fix, the stream is not getting assigned to the Response at all, and so `into_string()` is successfully reading the synthetic error body.

algesten: Thanks for this! If it isn't solved by the weekend, I'll have time to pitch in then. 
jsha: Fixed test; ready for review.

Patch:
diff --git a/src/response.rs b/src/response.rs
index 38f44f1..a768535 100644
--- a/src/response.rs
+++ b/src/response.rs
@@ -221,6 +221,11 @@ impl Response {
         &self.error
     }
 
+    // Internal-only API, to allow unit::connect to return early on errors.
+    pub(crate) fn into_error(self) -> Option<Error> {
+        self.error
+    }
+
     /// The content type part of the "Content-Type" header without
     /// the charset.
     ///
diff --git a/src/unit.rs b/src/unit.rs
index 3ec4794..2957af1 100644
--- a/src/unit.rs
+++ b/src/unit.rs
@@ -198,6 +198,8 @@ pub(crate) fn connect(
             let empty = Payload::Empty.into_read();
             return connect(req, unit, false, redirect_count, empty, redir);
         }
+        // Non-retryable errors return early.
+        return Err(resp.into_error().unwrap());
     }
 
     // squirrel away cookies


Test patch:
diff --git a/src/test/timeout.rs b/src/test/timeout.rs
index cb69055..7a296e4 100644
--- a/src/test/timeout.rs
+++ b/src/test/timeout.rs
@@ -50,7 +50,6 @@ fn overall_timeout_during_body() {
 // Send HTTP headers on the TcpStream at a rate of one header every 100
 // milliseconds, for a total of 30 headers.
 fn dribble_headers_respond(mut stream: TcpStream) -> io::Result<()> {
-    stream.write_all(b"HTTP/1.1 200 OK\r\nContent-Length: 0\r\n")?;
     for _ in 0..30 {
         stream.write_all(b"a: b\n")?;
         stream.flush()?;
@@ -59,12 +58,35 @@ fn dribble_headers_respond(mut stream: TcpStream) -> io::Result<()> {
     Ok(())
 }
 
+#[test]
+fn read_timeout_during_headers() {
+    let server = TestServer::new(dribble_headers_respond);
+    let url = format!("http://localhost:{}/", server.port);
+    let resp = crate::get(&url).timeout_read(10).call();
+    assert!(!resp.ok());
+    assert_eq!(
+        resp.into_string().unwrap(),
+        "Network Error: timed out reading response\n"
+    );
+}
+
 #[test]
 fn overall_timeout_during_headers() {
     // Start a test server on an available port, that dribbles out a response at 1 write per 10ms.
     let server = TestServer::new(dribble_headers_respond);
     let url = format!("http://localhost:{}/", server.port);
-    get_and_expect_timeout(url);
+    let agent = Agent::default().build();
+    let timeout = Duration::from_millis(500);
+    let resp = agent.get(&url).timeout(timeout).call();
+    assert!(
+        matches!(resp.synthetic_error(), Some(Error::Io(_))),
+        "expected timeout error, got {:?}",
+        resp.synthetic_error()
+    );
+    assert_eq!(
+        resp.synthetic_error().as_ref().unwrap().body_text(),
+        "Network Error: timed out reading response"
+    );
 }
 
 #[test]