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

src: do proper StringBytes error handling #12765

Closed
wants to merge 4 commits into from

Conversation

@addaleax
Copy link
Member

commented Apr 30, 2017

  • Return MaybeLocals from StringBytes::Encode
  • Add an error out parameter to pass JS exceptions to the callers (instead of directly throwing)
  • Simplify some of the string generation methods in string_bytes.cc by unifying the EXTERN_APEX logic
  • Reduce usage of deprecated V8 APIs.
  • Remove error handling logic from JS, the buffer.*Slice() methods now throw errors themselves.
  • Left TODO comments for future semver-major error message improvements.

This paves the way for better error messages coming out of the StringBytes methods.

Ref: #3175

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

buffer, fs, os, crypto


CI: https://ci.nodejs.org/job/node-test-commit/9528/

Maybe @TimothyGu or @tniessen would want to review?

Also, /cc @MylesBorins , if you’re still interested in #3175 … that should become a lot easier after this :)

lib/buffer.js Outdated
@@ -590,11 +589,8 @@ Buffer.prototype.toString = function(encoding, start, end) {

if (end <= start)

This comment has been minimized.

Copy link
@benjamingr

benjamingr Apr 30, 2017

Member

This is much clearer now, had to double check undefined <= undefined returns false due to number hint.

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 30, 2017

Author Member

@benjamingr Not sure, what are you saying? 😄 I didn’t really touch this part of the logic ;)

This comment has been minimized.

Copy link
@benjamingr

benjamingr Apr 30, 2017

Member

You added an early return which eliminates a correct (but very tricky) behavior in the old code where it would check if end <= start for undefined values. I'm just saying I'm happy with the change :)

src/node_crypto.cc Outdated
delete[] md_value;
args.GetReturnValue().Set(rc);
// TODO(addaleax): Throw `error` here. This isn't so terribly important,

This comment has been minimized.

Copy link
@benjamingr

benjamingr Apr 30, 2017

Member

So why not throw the error here?

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 30, 2017

Author Member

Hm, right, I could just do that here. I didn’t want to accidentally turn this into a semver-major change, but I guess here it really doesn’t make a difference… Updated!

MaybeLocal<Value> StringBytes::Encode(Isolate* isolate,
const uint16_t* buf,
size_t buflen,
Local<Value>* error) {

This comment has been minimized.

Copy link
@benjamingr

benjamingr Apr 30, 2017

Member

Personally, I would use a reference and not a pointer here - passing Isolate as a pointer makes sense since it's convention - but if the goal is to literally a const pointer to an object that might change (but not the pointer) I think a reference is cleaner. If this is convention and we can't do a reference - can we make it a const pointer?

(goes to all sites)

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 30, 2017

Author Member

Personally, I agree. But yes, we don’t use non-const references in this codebase, and I can live with that… ¯\_(ツ)_/¯

If this is convention - can we make it a const pointer?

Sorry, which thing should be made const? The Local can’t be const, Encode() needs to be able to assign to it. The pointer itself could be const, but it’s a pass-by-value function parameter, so that doesn’t really matter anyway, does it?

This comment has been minimized.

Copy link
@benjamingr

benjamingr Apr 30, 2017

Member

The pointer itself, so it's clear it doesn't change - but I don't feel strongly about this and it's plenty obvious it won't change (like the isolate).

src/string_bytes.cc Outdated

case BASE64: {
size_t dlen = base64_encoded_size(buflen);
char* dst = node::UncheckedMalloc(dlen);
if (dst == nullptr) {
return Local<String>();
goto malloc_failed;

This comment has been minimized.

Copy link
@benjamingr

benjamingr Apr 30, 2017

Member

I think a function containing the logic of malloc_failed and call sites calling the functions (rather than using goto is cleaner. I don't have a lot of experience with this code though.

This comment has been minimized.

Copy link
@addaleax

addaleax Apr 30, 2017

Author Member

I’ve just inlined the malloc_failed case, it was a lot bulkier and made more sense this way when I started editing. 😄

@benjamingr
Copy link
Member

left a comment

I have several nits:

  • Lots of new TODOs that can probably be fixed in this PR pretty quickly.
  • I think a reference is cleaner (or at least a const pointer)
  • Not a goto fan.

That said, I gave the code a read, understand what it does and the changes look good to me with or without the nits addressed - given we can always address them later nits addressed.

I'd feel more comfortable if someone more experienced with the code gave this a review too though :)

@addaleax

This comment has been minimized.

Copy link
Member Author

commented Apr 30, 2017

Lots of new TODOs that can probably be fixed in this PR pretty quickly.

I’d like to keep them for a quick semver-major follow-up PR after this lands, if that’s okay.

@benjamingr

This comment has been minimized.

Copy link
Member

commented Apr 30, 2017

@addaleax sounds good to me. Editing previous comment.

lib/buffer.js Outdated
if (arguments.length === 0) {
result = this.utf8Slice(0, this.length);
return this.utf8Slice(0, this.length);

This comment has been minimized.

Copy link
@cjihrig

cjihrig May 1, 2017

Contributor

By changing this to return, you can drop the else and save a level of indentation on the rest of the code below.

This comment has been minimized.

Copy link
@addaleax

addaleax May 1, 2017

Author Member

@cjihrig yup, done!

@jasnell
jasnell approved these changes May 1, 2017
addaleax added 4 commits Apr 30, 2017
src: do proper StringBytes error handling
- Return `MaybeLocal`s from `StringBytes::Encode`
- Add an `error` out parameter to pass JS exceptions to the callers
  (instead of directly throwing)
- Simplify some of the string generation methods in `string_bytes.cc`
  by unifying the `EXTERN_APEX` logic
- Reduce usage of deprecated V8 APIs.
- Remove error handling logic from JS, the `buffer.*Slice()` methods
  now throw errors themselves.
- Left TODO comments for future semver-major error message
  improvements.

This paves the way for better error messages coming out of the
StringBytes methods.

Ref: #3175

@addaleax addaleax force-pushed the addaleax:stringbytes-cleanup branch to 389af5a May 1, 2017

@tniessen

This comment has been minimized.

Copy link
Member

commented May 1, 2017

Nothing to add, looks good :)

@addaleax

This comment has been minimized.

Copy link
Member Author

commented May 3, 2017

@addaleax

This comment has been minimized.

Copy link
Member Author

commented May 3, 2017

Landed in d56a7e6

@addaleax addaleax closed this May 3, 2017

@addaleax addaleax deleted the addaleax:stringbytes-cleanup branch May 3, 2017

addaleax added a commit that referenced this pull request May 3, 2017
src: do proper StringBytes error handling
- Return `MaybeLocal`s from `StringBytes::Encode`
- Add an `error` out parameter to pass JS exceptions to the callers
  (instead of directly throwing)
- Simplify some of the string generation methods in `string_bytes.cc`
  by unifying the `EXTERN_APEX` logic
- Reduce usage of deprecated V8 APIs.
- Remove error handling logic from JS, the `buffer.*Slice()` methods
  now throw errors themselves.
- Left TODO comments for future semver-major error message
  improvements.

This paves the way for better error messages coming out of the
StringBytes methods.

Ref: #3175
PR-URL: #12765
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
src: do proper StringBytes error handling
- Return `MaybeLocal`s from `StringBytes::Encode`
- Add an `error` out parameter to pass JS exceptions to the callers
  (instead of directly throwing)
- Simplify some of the string generation methods in `string_bytes.cc`
  by unifying the `EXTERN_APEX` logic
- Reduce usage of deprecated V8 APIs.
- Remove error handling logic from JS, the `buffer.*Slice()` methods
  now throw errors themselves.
- Left TODO comments for future semver-major error message
  improvements.

This paves the way for better error messages coming out of the
StringBytes methods.

Ref: nodejs#3175
PR-URL: nodejs#12765
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 18, 2017

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.