fetch() fails when reusing persistent connection that has closed.

[NfNitLoop]

I've done some more investigation since #11495 and believe I've found the issue.

If a server unexpectedly closes a connection during a call to fetch(), the next call to fetch() will fail.

This seems like a bug for 2 reasons:

1. Deno should probably check that the existing persistent connection is still valid (connected) before attempting to re-use it.
2. This behavior makes it really difficult to debug which fetch() is causing an error.

Steps to reproduce

Run this code against a server that closes a connection early.

const port = 9999

const brokenURL = `http://127.0.0.1:${port}/`

// If the server correctly reads all of the body, then there's no problem:
const fixedURL = `http://127.0.0.1:${port}/?drain=true`

// Bigger files seem to trigger this more reliably:
const bigFile = new Uint8Array(1024*1024*50)

async function get(): Promise<void> {
    // GETs always work OK because there's no body.
    const r = await fetch(brokenURL)
    if (!r.ok) throw {result: r, status: r.status, statusText: r.statusText}
}

async function put(url: string) {
    const r = await fetch(url, { method: "PUT", body: bigFile })
    if (!r.ok) throw {result: r, status: r.status, statusText: r.statusText}
    console.log("Put finished")
}

// These work fine:
await get()
await get()
await get()
await get()
await get()
await get()

// This causes the keepalive connection to error, but in such a way that this
// call to Deno's fetch() still succeeds
await put(brokenURL)

// This tries to reuse the keepalive connection that has been closed and dies.
// IMO, this is a bug. As with a connection pool, it's best to make sure the
// existing connection is still valid before trying to reuse it.
await get()

I've reproduced this behavior in Actix-Web and Tide. Yes, it's likely best behavior to always read the body, but this is an easy error to make and likely a case that folks writing Deno scripts will run into again. I'm hoping to spare them my headaches. :)

Actix-Web

use std::io;

use actix_web::{App, HttpResponse, HttpServer, web::{Payload, Query, route}};
use futures::StreamExt;
use serde::Deserialize;

/// Responds to an HTTP request before the full body has been read. 
/// In Actix-web, this causes the conneciton to be dropped before the
/// client expects, which causes difficult-to-debug behavior in Deno's
/// fetch().
///
async fn fast_responder(
    Query(params): Query<Params>,
    mut req_body: Payload,
) -> HttpResponse {

    // If I wait for the full body *before* sending a response, things work:
    if params.drain.unwrap_or(false) {
        while req_body.next().await.is_some() {}
    }

    HttpResponse::Ok().finish()
}

fn main() -> io::Result<()>{
    let port = 9999;
    let server = HttpServer::new(|| {
        App::new().configure(|cfg| {
            cfg.route("/", route().to(fast_responder));
        })
    }).bind(format!("127.0.0.1:{}", port))?;

    let mut system = actix_web::rt::System::new("web server");
    system.block_on(server.run())?;

    Ok(())
}

#[derive(Deserialize)]
struct Params {
    drain: Option<bool>
}

Tide

use futures::AsyncReadExt;
use tide::Request;
use tide::prelude::*;

async fn fast_responder(mut req: Request<()>) -> tide::Result {
    let params: Params = req.query()?;

    if params.drain.unwrap_or(false) {
        let mut reader  = req.take_body().into_reader();
        let mut buf = [0u8; 1024*16];
        while reader.read(&mut buf).await? > 0 {}
    }

    Ok("".into())
}

#[async_std::main]
async fn main() -> tide::Result<()> {
    let mut app = tide::new();
    app.at("/").get(fast_responder).put(fast_responder);
    app.listen("127.0.0.1:9999").await?;
    Ok(())
}

#[derive(Deserialize)]
struct Params {
    drain: Option<bool>
}

[lucacasonato]

Possibly related to seanmonstar/reqwest#1276

[jkingry]

Possibly related to seanmonstar/reqwest#1276

Possibly, but it's been closed by this PR and perhaps that did not resolve the issue.

I'm also experiencing this issue, and I suspect it's around connection re-use as it happens when multiple fetches are in-flight.

[NfNitLoop]

Still experiencing this issue in new Deno code I'm writing in 2.0.

In this case, the behavior I'm seeing is:

• use fetch() to read some data in a long running process.
• Walk away from the computer for a while. (no fetches are happening for a while.)
• Come back and prod the long-running process to do another fetch to the same server.

The first fetch gets a "connection reset" error:

 TypeError: error sending request from 10.1.1.16:59612 for https://blog.nfnitloop.com//homepage/proto3? (162.243.253.171:443): client error (SendRequest): connection error: connection reset
    at mainFetch (ext:deno_fetch/26_fetch.js:182:11)
    at eventLoopTick (ext:core/01_core.js:175:7)
    at async fetch (ext:deno_fetch/26_fetch.js:392:7)
…

But subsequent fetches work as expected.

My theory is that Deno is trying to keep the connection open for re-use, but the connection gets closed (or NAT rules expire) before the next use.

This should be handled transparently for users. If re-using a connection fails because it had been idle too long, that's out of users' control.

In the meantime, I'm just going to need to wrap fetch() and retry on this case.