`.json()` ignores handler errors

[jasonk]

trafficstars

Describe the bug

• Node.js version: v16.14.0
• OS & version: macOS Monterey (12.6.1)

The documentation page includes an example of a handler that throws a custom error to replace the original error. This example does not work and any error you throw from within a handler will leak (unless you have an unhandledRejection listener to do something with it).

I tested this with a handler that is pretty much identical to the recommendation in the docs:

    const got = Got.extend( { handlers : [
      ( options, next ) => {
        return ( async () => {
          try {
            return await next( options );
          } catch ( error ) {
            throw new Error( 'Replacement Error' );
          }
        } )();
      },
    ] } );
    await got( `https://httpstat.us/404` ).json();

Actual behavior

Throwing an error from a handler leaks a promise that will be rejected with the new error, and has no effect on the result of the request. If the actual HTTP request was successful then the got request will be resolved successfully, ignoring the error that was thrown in the handler. If the actual HTTP request was unsuccessful then any error you throw from the handler will again be ignored, and the got request will be resolved with whatever the original failure was.

Running the script included below produces this:

Attempting to replace error
[FAIL] Got wrong error: Response code 404 (Not Found)
[FAIL] Got unhandled rejection: Replacement Error

Throwing error on successful request
[FAIL] Did not expect a successful response
[FAIL] Got unhandled rejection: Async Error

Expected behavior

Throwing an error from a handler causes the request to be rejected with that error.

I expected that the script below would produce this:

Attempting to replace error
[PASS] Got correct error

Throwing error on successful request
[PASS] Got correct error

Code to reproduce

Here is a more complete and runnable test script

#!/usr/bin/env node
import Got from 'got';

process.on( 'unhandledRejection', err => {
  console.log( `[FAIL] Got unhandled rejection: ${err.message}` );
} );

async function replaceError() {
  try {
    const got = Got.extend( { handlers : [
      ( options, next ) => {
        return ( async () => {
          try {
            return await next( options );
          } catch ( error ) {
            throw new Error( 'Replacement Error' );
          }
        } )();
      },
    ] } );
    return await got( `https://httpstat.us/404` ).json();
  } catch ( err ) {
    if ( err.message === 'Replacement Error' ) {
      console.log( '[PASS] Got correct error' );
    } else {
      console.log( '[FAIL] Got wrong error:', err.message );
    }
  }
}

async function throwOnSuccessfulRequest() {
  try {
    const got = Got.extend( { handlers : [
      ( options, next ) => {
        return ( async () => {
          await next( options );
          throw new Error( 'Async Error' );
        } )();
      },
    ] } );
    await got( `https://httpstat.us/200` ).json();
    console.log( '[FAIL] Did not expect a successful response' );
  } catch ( err ) {
    if ( err.message === 'Async Error' ) {
      console.log( '[PASS] Got correct error' );
    } else {
      console.log( '[FAIL] Got wrong error:', err.message );
    }
  }
}

// This interval just makes sure that node keeps the script running
// until all the promises are resolved.
const interval = setInterval( () => {}, 1000 );

console.log( 'Attempting to replace error' );
await replaceError();

// This is just to make sure that the unhandled rejection is detected
// before we move on to the next test
await new Promise( resolve => setTimeout( resolve, 1000 ) );

console.log( '\nThrowing error on successful request' );
await throwOnSuccessfulRequest();

clearInterval( interval );

Checklist

• [x] I have read the documentation.
• [x] I have tried my code with the latest version of Node.js and Got.

[jasonk]

I have been attempting to track down the cause of this but haven't had much luck so far. I did notice that the iterateHandlers function that is actually running the handlers is disabling the @typescript-eslint/no-floating-promises rule which strengthens my hunch that the problem is there, but I don't yet fully understand how that and asPromise are interacting when running these handlers.

[szmarczak]

The issue is that you call .json() which returns a promise that does not depend on its parent.

Indeed, this is a Got bug. However the current approach is required since the user is in control of the type of the result (responseType) of the parent promise.

The issue should be fixed once the responseType option gets removed.