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

feat(DataResolver): prefer streams over buffers #4075

Merged
merged 6 commits into from
Apr 17, 2020

Conversation

jakeweary
Copy link
Contributor

@jakeweary jakeweary commented Apr 12, 2020

Please describe the changes this PR makes and why it should be merged:

I've noticed high memory usage when tried to send a few ~100mb files as message attachments so I went to the source code and discovered that, indeed, everything is being converted to Buffers for some reason. Since discord.js uses node-fetch under the hood, which accepts Streams, the fix seems to be pretty simple.

Status

  • Code changes have been tested against the Discord API, or there are no code changes
  • I know how to update typings and have done so, or typings don't need updating

Semantic versioning classification:

  • This PR changes the library's interface (methods or parameters added)
    • This PR includes breaking changes (methods removed or renamed, parameters moved or removed)
  • This PR only includes non-code changes, like changes to documentation, README, etc.

@SpaceEEC SpaceEEC linked an issue Apr 12, 2020 that may be closed by this pull request
Add `resolveFileAsBuffer` to use it in `resolveImage` which still requires Buffers to work.
@jakeweary
Copy link
Contributor Author

Uh, don't merge, getting DiscordAPIError: Cannot send an empty message when trying to send some mp4 by url.

src/util/DataResolver.js Show resolved Hide resolved
src/util/DataResolver.js Outdated Show resolved Hide resolved
@jakeweary jakeweary marked this pull request as draft April 14, 2020 09:38
@jakeweary
Copy link
Contributor Author

Some progress on the DiscordAPIError: Cannot send an empty message issue:

const https = require('https');
const FormData = require('form-data');
const fetch = require('node-fetch');

const httpbin = async (key, value) => {
  const body = new FormData();
  body.append(key, value);
  const res = await fetch('http://httpbin.org/post', { body, method: 'POST' });
  console.log(await res.json());
};

(async () => {
  const url = 'https://discord.js.org/index.html';

  // works, Content-Length header is empty
  await httpbin('index.html', await new Promise(ok => https.get(url, ok)));

  // doesn't work, Content-Length is being set to some numeric value
  await httpbin('index.html', await fetch(url).then(r => r.body));
})();

@jakeweary
Copy link
Contributor Author

Alright, found it: form-data/form-data#382 fixes the issue I'm having here in this PR. Unfortunately, it doesn't seem like it will be merged in near future.

In a nutshell: there's a bug in form-data so it can't handle all streams in the same way. Streams returned by fs.createReadStream are handled fine, as well as streams returned by http.get and https.get, but other than these, no streams can be considered safe to use with the current version of form-data.

@jakeweary jakeweary marked this pull request as ready for review April 16, 2020 11:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TypeError: Cannot read property 'pipe' of undefined
6 participants