Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Always uses an in-memory buffer, never allows streaming to disk #41

Open
Stephen-Meyerhofer opened this issue Sep 4, 2016 · 1 comment

Comments

@Stephen-Meyerhofer
Copy link

Stephen-Meyerhofer commented Sep 4, 2016

I wanted to try using this module instead of request because it would handle ESOCKETTIMEDOUT (and more) errors for me. However, there is a big issue when it comes to streaming.

The bl module uses Node's buffer.js. Node buffers have a limit of 256MB. I get the error

Uncaught Exception:
Error: "toString()" failed
...

when I use this module, even though only pass in the options object as an argument to requestretry.

Here is the culprit in your code. I may fork and fix the code myself, but I am not sure if I have enough time to think of all the things I might break for other users of this module.

this._req = Request.request(this.options, function (err, response, body) {
    if (response) {
      response.attempts = this.attempts;
    }
    if (this.retryStrategy(err, response, body) && this.maxAttempts > 0) {
      this._timeout = setTimeout(this._tryUntilFail.bind(this), this.retryDelay);
      return;
    }

    this.reply(err, response, body);
  }.bind(this));

A solution, I think, is to listen to the .on( 'end' ... event instead of passing in the callback to request. Of course, sending the callback is fine IF AND ONLY IF the user passes a callback in as well. The callback destroys the opportunity we have to use the excellent streaming interface on the request module.

When downloading large files (and in my case, multiple large files. Perhaps several over 1GB), this module is impossible to use. Once we pass a callback to request, we are telling request to buffer data in memory. By using the streaming interface, we are able to download several huge files because the data goes in and out of memory (drains) as fast as possible.

--- Want to back this issue? **[Post a bounty on it!](https://www.bountysource.com/issues/37570683-always-uses-an-in-memory-buffer-never-allows-streaming-to-disk?utm_campaign=plugin&utm_content=tracker%2F1861872&utm_medium=issues&utm_source=github)** We accept bounties via [Bountysource](https://www.bountysource.com/?utm_campaign=plugin&utm_content=tracker%2F1861872&utm_medium=issues&utm_source=github).
@FGRibreau
Copy link
Owner

@Stephen-Meyerhofer indeed, I perfectly understand your issue here. Since we have test-coverage you may submit a PR that change this behaviour :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants