tomas / needle

Nimble, streamable HTTP client for Node.js. With proxy, iconv, cookie, deflate & multipart support.
https://www.npmjs.com/package/needle
MIT License
1.63k stars 236 forks source link

needle streams and handling errors #303

Open dodtsair opened 4 years ago

dodtsair commented 4 years ago

It is not clear from the documentation how would one handle errors while attempting to stream.

Consider the following example code

const Q = require('q');
const needle = require('needle');
const bufferingArray = []

const qPromise = new Promise((resolve, reject) => {
  const stream = needle.get('https://httpstat.us/404', {}, function callback(error, response) {
    if(response.statusCode === 404) {
      reject(response);
    }
  });
  stream.once('readable', chunk => {
    resolve(stream);
  });
  stream.once('error', error => {
    console.log("Stream error: ", error);
  });
})

qPromise.then(stream => {
  let data = [];
  stream.on('data', chunk => {
    data.push(chunk)
  });
  stream.once('end', () => {
    console.log("Response :", Buffer.concat(data).toString())

  });
  stream.once('error', (error) => {
    console.log("Error: ", error);
  })
  stream.once('err', (error) => {
    console.log("Err: ", error);
  })

}).catch(error => {
  console.log("Promise error: ", error)
})

This client using needle wants to treat 404 as an error. Imagine this is supposed to download an image. Since the request for the image is a 404 the client will want to have error handling code kick in. However since needle does not treat 404s as an error, the response body of the 404 response is done first, before the callback is called. This example shows that the console.log will output the body of the 404 response before the 404 status code can be checked in the callback.

There is no way to tell from the stream that the request is an error. Which means the client treats the 404 response as an image and blows up

What is important is to change the code to use on('response',...) and trigger error handling at this point.

Consider the new example:

const qPromise = new Promise((resolve, reject) => {
  const stream = needle.get('https://httpstat.us/200');
  stream.once('response', response => {
    if(response.statusCode === 404) {
      reject(response);
    } else {
      resolve(stream);
    }
  });
})

qPromise.then(stream => {
  let data = [];
  stream.on('data', chunk => {
    data.push(chunk)
  });
  stream.once('end', () => {
    console.log("Response :", Buffer.concat(data).toString())

  });
  stream.once('error', (error) => {
    console.log("Error: ", error);
  })
  stream.once('err', (error) => {
    console.log("Err: ", error);
  })

}).catch(error => {
  console.log("Promise error: ", error)
})

In this example switching the URL back and forth between https://httpstat.us/200 and https://httpstat.us/404 has the expected results. For a 200 the body is processed by the client, for 404 the client receives an error and can do error handling.

tomas commented 4 years ago

How would you update the current implementation to handle this correctly?