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

Don't allocate buffer twice #1403

Merged
merged 3 commits into from Sep 14, 2020
Merged

Don't allocate buffer twice #1403

merged 3 commits into from Sep 14, 2020

Conversation

xamgore
Copy link
Contributor

@xamgore xamgore commented Aug 11, 2020

Now a new Buffer created without copying the underlying memory.
https://nodejs.org/api/buffer.html#buffer_class_method_buffer_from_arraybuffer_byteoffset_length

Checklist

  • I have read the documentation.
  • I have included a pull request description of my changes.
  • I have included some tests.
  • If it's a new feature, I have included documentation updates.

* getBuffer returns a buffer of concatenated response chunks.
* Its result is stored in rawBody and passed to parseBody.
* Then it meets Buffer.from(rawBody) and triggers a new buffer allocation:
   https://nodejs.org/api/buffer.html#buffer_class_method_buffer_from_buffer

Now a new Buffer created without copying the underlying memory.
https://nodejs.org/api/buffer.html#buffer_class_method_buffer_from_arraybuffer_byteoffset_length
@sindresorhus
Copy link
Owner

Tests are failing.

@xamgore
Copy link
Contributor Author

xamgore commented Aug 22, 2020

Yeah, I know. Just need time to get familiar with the project and tests.

@szmarczak
Copy link
Collaborator

parseBody copies the body so even if you modify the buffer then promise.text() will give the original body.

@szmarczak szmarczak closed this Sep 13, 2020
@szmarczak szmarczak reopened this Sep 13, 2020
@szmarczak
Copy link
Collaborator

szmarczak commented Sep 13, 2020

@sindresorhus Although we could add an environment variable to disable this.

		if (responseType === 'buffer') {
			return process.env.GOT_MUTABLE_BODY ? rawBody : Buffer.from(rawBody);
		}

or should we just return the original buffer? If so, then this could happen:

const promise = got('https://somesitehere.com'); //=> 123
const buffer = await promise.buffer();
buffer.write(49, 0);

const text = await promise.text(); //=> 223, not 123!

or we could warn users in the docs... If we decide to return the original buffer then it will use 2x less RAM for people who do got().buffer().

@sindresorhus
Copy link
Owner

or should we just return the original buffer? If so, then this could happen:

That seems extremely unlikely though. Why would they write to a buffer they got from a method call. But maybe we could just override the .write() method and throw an error if used?


Also, upvote and comment on this issue: nodejs/node#27080

@szmarczak szmarczak merged commit 7bc69d9 into sindresorhus:master Sep 14, 2020
@sindresorhus
Copy link
Owner

But maybe we could just override the .write() method and throw an error if used?

@szmarczak Thoughts on this?

@szmarczak
Copy link
Collaborator

Thoughts on this?

That wouldn't work as you'd still be able to do buffer[0] = 123; since it's also Uint8Array...

@sindresorhus
Copy link
Owner

We could Object.freeze it?

@szmarczak
Copy link
Collaborator

TypeError: Cannot freeze array buffer views with elements

@sindresorhus
Copy link
Owner

@sindresorhus
Copy link
Owner

@szmarczak Maybe we could at least wrap the TS type in Readonly so at least TS users are protected?

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.

None yet

3 participants