Got: `stream.aborted` is always `true` when using cache

[szmarczak]

cacheable-request incorrectly mimics the response

[Giotino]

Thanks to the great Node.JS documentation the only way to know what does request.aborted and request.destroyed means is to look at the source code.

If you look here
https://github.com/nodejs/node/blob/master/lib/_http_client.js#L407-L433
destroyed is true every time the socket is closed (even with a complete response), aborted is true only when the response is incomplete (!res.complete).
I think we should use this[kRequest]?.aborted instead of this[kRequest]?.destroyed in get aborted()

[Giotino]

this[kRequest]?.aborted type from DefinitelyTyped is number, but according to the Node.JS documentation it's a boolean.

Looking at this PR https://github.com/DefinitelyTyped/DefinitelyTyped/pull/38291 it seems that in Node.JS 11 it has been changed from

If a request has been aborted, this value is the time when the request was aborted, in milliseconds since 1 January 1970 00:00:00 UTC.

to

The request.aborted property will be true if the request has been aborted.

[szmarczak]

But the issue is with cacheable-request, not Node.js.

[Giotino]

But the issue is with cacheable-request, not Node.js.

I don't think so. It's true that it only happens while using the cache but that's because it's the only time

let requestOrResponse = await fn(url, requestOptions);

is a request. It's a ClientRequest and it's stored into this[kRequest] that's checked for the aborted getter.

If you use cacheable-request alone (see example below) it has the same behavior as the wrapped function.

// Results may change due to hardware/network performance differences
const http = require('http');
const CacheableRequest = require('cacheable-request');

const cacheableRequest = new CacheableRequest(http.request);
const cacheReq = cacheableRequest('http://example.com');

cacheReq.on('request', req => {
  req.end();
  console.log('cacheableRequest', req.destroyed); // false
  setTimeout(() => {
    console.log('cacheableRequest +200ms', req.destroyed); // false
  }, 200);
  setTimeout(() => {
    console.log('cacheableRequest +500ms', req.destroyed); // true
  }, 500);
  setTimeout(() => {
    console.log('cacheableRequest +1000ms', req.destroyed); // true
  }, 1000);
});


const normalReq = http.request('http://example.com');

normalReq.end();
console.log('normalRequest', normalReq.destroyed); // false
setTimeout(() => {
  console.log('normalRequest +200ms', normalReq.destroyed); // false
}, 200);
setTimeout(() => {
  console.log('normalRequest +500ms', normalReq.destroyed); // true
}, 500);
setTimeout(() => {
  console.log('normalRequest +1000ms', normalReq.destroyed); // true
}, 1000);

[szmarczak]

If you use cacheable-request alone (see example below) it has the same behavior as the wrapped function.

How does this relate to the issue? You just proved that request.destroyed is the same.

https://github.com/sindresorhus/got/blob/b134aa488a3d903d9571914b0f8b267340b767a9/source/core/index.ts#L2688

complete is missing and the request is destroyed, so aborted always returns true.

[Giotino]

I might be missing something.

Doing a little recap:
In the first message you said that

cacheable-request incorrectly mimics the response

but with my ugly example from the previous message I can see that "stream.aborted is always true when using cache" is true even without cacheable-request.

I admit that in my previous messages I made some confused statement throwing random "aborted" and "destroyed" everywhere without specifying the object I was referring to.

Let me sum up:
We are talking about the stream API.
Actually stream.aborted is not always true, it's true after some time, but let say that's always true (it doesn't change much).

When using the cache stream.aborted is always true because this[kRequest]?.destroyed is always true.
https://github.com/sindresorhus/got/blob/b134aa488a3d903d9571914b0f8b267340b767a9/source/core/index.ts#L2688

this[kRequest]?.destroyed become true when the writing stream (for the request) to the server is closed (even when the request was successfully written to the server).
That's not good, because got.stream().aborted specify when a request was aborted (i.e. ended before it's completed).
In order to preserve the meaning of got.stream().aborted I think this[kRequest]?.aborted should be used because it's true only when the writing stream is closed and the request wasn't fully written to the server.

BTW: If you change this[kRequest]?.destroyed with this[kRequest]?.aborted the issue disappear and all the tests are ok.

[szmarczak]

but with my ugly example from the previous message I can see that "stream.aborted is always true when using cache" is true even without cacheable-request.

How come if you don't refer to gotStream.aborted nor clientRequest.aborted at all?

Actually stream.aborted is not always true

It's always true if the response has been consumed.

I think this[kRequest]?.aborted should be used

No, because see the very first comment in this issue:

cacheable-request incorrectly mimics the response

Exactly. It doesn't proxy the [response] complete ~nor aborted~ property.

[szmarczak]

request.aborted is not used because when using the cache you don't receive a ClientRequest instance but a IncomingMessage one.

[Giotino]

Ok, found what I was missing, we were talking about 2 different things, my bad.

But I found the root cause of the issue: https://github.com/lukechilds/cacheable-request depends on https://github.com/lukechilds/clone-response which depends on https://github.com/sindresorhus/mimic-response v1.1.0 that has the "complete bug".

If you upgrade to mimic-response v3.1.0 (latest release) it marks the cloned response as completed but it fails on this check.

if (toStream._readableState.autoDestroy) {
  throw new Error('The second stream must have the `autoDestroy` option set to `false`');
}

But I found a solution (easy no brainer, needs to be reviewed): on https://github.com/lukechilds/clone-response just new PassThrough(); => new PassThrough({autoDestroy: false});

[Giotino]

I've setup a fork of cacheable-request (with a fixed fork of clone-response), try the master branch https://github.com/Giotino/cacheable-request

[szmarczak]

It won't work, because it also checks if the Got stream is destroyed. And we cannot disable autoDestroy in Got because it would break some tests.

[Giotino]

It won't work, because it also checks if the Got stream is destroyed. And we cannot disable autoDestroy because it would break some tests.

Ok, that was the reason because I wanted some review, I was worried that it was needed.

[Giotino]

And we cannot disable autoDestroy in Got because it would break some tests.

503 tests passed
1 known failure
1 test skipped

Tests seems fine, I think it's because of that

response.once('error', (error: Error) => {
    // Force clean-up, because some packages don't do this.
    // TODO: Fix decompress-response
    response.destroy();

    this._beforeError(new ReadError(error, this));
});

Here you basically implemented the autoDestroy yourself.

The stream is closed when the 'error' event is emitted unless the autoDestroy option was set to false when creating the stream.

That "fix" is there because decompress-response does the same thing with mimic-response as I suggested with clone-response

[szmarczak]

Tests seems fine

Hmmm, that's weird.

Here you basically implemented the autoDestroy yourself.

Yep, I cannot recall why I did this. But I know Node.js recommends enabling autoDestroy.

[Giotino]

Is there a reason why

The second stream must have the autoDestroy option set to false

in mimic-response?

[szmarczak]

Is there a reason why

I don't remember exactly what, but feel free to check the tests.

But anyway, cacheable-request needs to be rewritten and I plan to do so but I haven't got to it yet.

[Giotino]

I don't remember exactly what, but feel free to check the tests.

There are no tests regarding autoDestroy except the one that checks the exception.

By setting autoDestroy to true all the tests pass successfully.

[Giotino]

Found something in the PR

Regarding the .destroy(), I didn't do it because I'd have to do this:

stream._originalDestroy = stream._destroy;
stream._destroy = (error, callback) => {
  response.destroy();

  stream._originalDestroy(error, callback);
};

and the stream must be autoDestroy: false anyway.

  const stream = new PassThrough({
      autoDestroy: false,
      destroy(error, callback) {
          response.destroy();

          callback(error);
      }
  });

The above looks much better than replacing stream._destroy.

The only limitation after this PR is to implement the destroy() function: #6 (comment) [the comment is the quote before]

But people should do it on their own as replacing stream._destroy is ugly, not safe and people need to pass {autoDestroy: false} anyway.

[szmarczak]

What PR is that?

[Giotino]

6 and 7 from https://github.com/sindresorhus/mimic-response

https://github.com/sindresorhus/mimic-response/pull/6
https://github.com/sindresorhus/mimic-response/pull/7

The 7 is where you added

if (toStream._readableState.autoDestroy) {
  throw new Error('The second stream must have the `autoDestroy` option set to `false`');
}

[szmarczak]

cacheable-request uses clone-response, which uses mimic-response@^1.0.0. The latest version of mimic-response (which fixes the issue) is 3.1.0.

[szmarczak]

This causes to ignore the response at this point

https://github.com/sindresorhus/got/blob/49c16ee54fb19ea7aa77e24ac8c2b602f0aad265/source/as-promise/index.ts#L47-L50

and results in #1522

[szmarczak]

@adityapatadia figured out that passing keep alive agents like this

const agent = {
  http: new http.Agent({ keepAlive: true }),
  https: new https.Agent({ keepAlive: true })
};

reproduces the issue :clap:

[szmarczak]

@adityapatadia This workaround should fix things:

  const response = await got.get("...", requestOptions).on('request', req => {
      req.prependOnceListener('cacheableResponse', cacheableResponse => {
        const fix = () => {
            if (!cacheableResponse.req) {
                return;
            }

            cacheableResponse.complete = cacheableResponse.req.res.complete;
        };

        cacheableResponse.prependOnceListener('end', fix);
        fix();
      });
  });

[adityapatadia]

Cool. I am using my own version of cacheable-response so I can use latest version of mimic-response.

@szmarczak thanks a lot for quickly figuring this out.

[adityapatadia]

On the other note, reliance on such abandoned packages should be reduced or @lukechilds should give others permission to maintain these packages.

AsI said before these become production issues for our company and we are ready to offer our axe if rewrite of cacheable request is to be done.

[szmarczak]

reliance on such abandoned packages should be reduced

You mean we should drop all of them? Because there is only one :)

or @lukechilds should give others permission to maintain these packages.

He did :) So he's not the one to blame here. I am. I haven't had the time to fix this yet.

if rewrite of cacheable request is to be done.

I have plans to rewrite cacheable-request fully. So we can just plug it in as the request option. It will be a Promise returning ClientRequest / IncomingMessage.

[adityapatadia]

Okay. It would help to release new version of clone-response.

[adityapatadia]

Btw, I can confirm this fix has resolved the problems.

[adityapatadia]

I would really appreciate if we can get somehow released a new version for cacheable-request with this fix. It's really hurting our prod environment.

[adityapatadia]

@szmarczak I would love to discuss / sponsor development of cacheable-request. It will not only help us but also broader community.

[szmarczak]

/cc @lukechilds

[lukechilds]

Sorry I don't have any time to work on this right now, my time is fully occupied with @getumbrel.

Please feel free to take the sponsorship @szmarczak if it's something you're interested in.

[szmarczak]

@lukechilds I'd love to. I'll start working on this in two weeks. This week and the next one I'm having uni exams. Could you give me a write permission on GitHub (master is marked as protected) and a publish on NPM or should I create a new package?

[lukechilds]

Sure!

Added you to the npm package and removed the write protection on master. (although tbh I always like to not write to master and instead only work in feature branches and squash merge everything in)

[szmarczak]

although tbh I always like to not write to master and instead only work in feature branches and squash merge everything in

Thanks, will definitely follow this!

@adityapatadia The next two weeks I'm having exam session. I'll let you know when I start working on this.