Memory leak when body is not used

I wanted to test a URL for validity; verifying that it returns a 200 response.

Using this code:

const fetch = require('node-fetch');

module.exports = (url) => fetch(url)
    .then(res => res.status === 200 && url);

...I found it caused a memory leak. Node that I"m not calling .body() or .json(). Profiling the memory usage over time, I was suspicious that lots of stream objects were being keep in memory. I speculatively added {method: 'HEAD'} to the options (which is of course the right thing to do in this case anyway), which fixed the memory leak.

Is this a bug in node-fetch? Should memory be freed whether or not the body of the response is ever used? If it's not a bug, I wonder if the documentation should make it clear that responses with a body must be cleared in some way to free memory?

Thanks for a great library!

Thx for reporting this, how severe is the leak?

It got pretty bad:

I will need to actually spend some time researching this, just to be sure, could you be hitting this problem?

http://stackoverflow.com/questions/31957979/node-js-twitter-streaming-memory-leak

since the garbage collection happens around 1.4GB

@richard-still-ft

I don't know enough about it I'm afraid. @richard-still-ft or @quarterto - can you help?

Side note: I was working on some other project today and find that other project, such as co-busboy use module like black-hole-stream to drain the body, which is probably another solution if you don't like HEAD request for some reasons.

I found another nasty bug when body is not being used

I'm sending a If-Modified-Since header
and get back a 304 Not modified

let res = await fetch(url)

if (res.status == 304)
    return // do nothing with the response, don't bother fetching the rest
else
    return res.text()

events.js:154
      throw er; // Unhandled 'error' event
      ^

Error: unexpected end of file
    at Zlib._handle.onerror (zlib.js:363:17)

And it exit the process and dosen't reject... Even tried wrapping it in try catch...

Question: (shouldn't) isen't the pipelined paused automatically when using fetch() and resumed when calling .text() .json() .blob() .arraybuffer()? (referring to browsers version)

I mean: It shouldn't download anything and waist bandwith if I don't use the body?

@jimmywarting

@jimmywarting you do want to use HEAD here, if you explicit don't need body. I don't think it's node-fetch's responsibility to ignore body if you don't give it clear instruction that you don't want body.

Plus the underlying native http.request might not be able to do that neither.

@jimmywarting

@jimmywarting as for the 304 example, I need to see a working native http.request code example to confirm something weird is happening and we can fix it somehow, or maybe a failed test cases for node-fetch.

@bitinn

@bitinn I Was able to reproduce every time with latest version and nothing else

I don't explicit need body. I conditionally need the body, that a different between sending a If-modified-since header and doing HEAD request. Otherwise i would have to do 2 request to see if there is anything to download

const fetch = require('node-fetch')

fetch('http://feber.se/rss', {headers: {
    'If-Modified-Since': (new Date()).toGMTString()
}}).then(res => {
    console.log(res.status)
    res.text() // without this line of code this will crash...
    // I could expect a rejection, but exiting the process?!?!?
    // Not even try/catch solves this

    // But a rejection is not the right way to do it either,
    // You should be able to call res.text() later as well (Making a breakpoint)
    // Now, calling .text() and download everything isn't a problem for me
    // cuz the response body is just like a HEAD response - Empty
    // But this is an unexpected behavior that you HAVE to call .text() to avoid a crash
})

And I can assure you that there is nothing wrong with http.request or the browser version of fetch

var http = require('http')
var options = {
  hostname: 'feber.se',
  path: '/rss',
  headers: {
    'If-Modified-Since': (new Date()).toGMTString()
  }
}

var req = http.request(options, (res) => {
  console.log('STATUS: ' + res.statusCode)
  console.log('HEADERS: ' + JSON.stringify(res.headers))
})
req.end()

This leads to the following conclusion that node-fetch keep downloading the body and doing something with it after without me explicit saying so.

https://fetch.spec.whatwg.org/#garbage-collection

6.7 Garbage collection
The user agent can terminate the fetch because the associated body is not observable.
window.promise = fetch("https://www.example.com/").then(res => res.headers)

There has to be something from stoping it from downloading everything if I don't use the body?

@jimmywarting

@jimmywarting I will need to run your code when I am available because I don't see why it should crash node-fetch, if it crashes without fulfilling then it's likely a bug.

As for your quote of fetch spec, I disagree:

To perform a fetch using request, run the steps below. An ongoing fetch can be terminated with reason reason, which must be end-user abort, fatal, timeout, or garbage collection.

garbage collection is not done through end user command, meaning, you don't get to pick when browser gc fetch request and body, nor how.

window.promise = fetch("https://www.example.com/").then(res => res.headers)

window.promise is a lot more important than a variable here because we are talking about gc. When it says observe, it means to avoid end user run into a state where browser has already garbage collected the request/body stream, not the reverse (where you would like the environment to explicit abort a request)

If there is anything unclear about fetch spec, do raise it as question there:

https://github.com/whatwg/fetch/issues

Again you are putting multiple issues in a thread that's has nothing to do with your actual issue (you are not experiencing memory leak). If you are serious about solving this, I will need you to make your point in another issue.

I am hoping as of v1.6.0, memory leaks should be less of a issue, if you still encounter it, use HEAD requests or pipe your body to something like https://github.com/meaku/black-hole-stream

https://github.com/node-fetch/node-fetch/issues/83

Navigation Menu

Repository navigation

Memory leak when body is not used #83

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Memory leak when body is not used #83

Description

Activity

bitinn commented on Mar 3, 2016

georgecrawford commented on Mar 3, 2016

bitinn commented on Mar 3, 2016

georgecrawford commented on Mar 3, 2016

bitinn commented on Mar 31, 2016

jimmywarting commented on Apr 16, 2016

jimmywarting commented on Apr 16, 2016

bitinn commented on Apr 17, 2016

bitinn commented on Apr 17, 2016

jimmywarting commented on Apr 17, 2016

jimmywarting commented on Apr 17, 2016

bitinn commented on Apr 25, 2016

bitinn commented on Aug 3, 2016

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions