src: use proper errors as coming from StringBytes by addaleax · Pull Request #14579 · nodejs/node · GitHub

The previous errors were incorrect here, as the code
only failed in situations where strings exceeded size limits or
an OOM situation was encountered, not for invalid encodings
(which aren’t even detected explicitly).

Unfortunately, these situations are hard to test for.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

src

return env->isolate()->ThrowException(error);? The call to rc.ToLocalChecked() two lines down will fail if you fall through.

Same issue appears a few more times further down in this file.

done

Stylistic change.

fixed up, this was not intentional

Suggestion: move the .IsEmpty() checks to after the StringBytes::Encode() calls, then you don't have to do the goto jump-around thing.

error is cleared by StringBytes::Encode() if the call succeeded… I’ve thought about doing that, would you prefer to change that behaviour? I can see the advantage that would have.

Good idea. I was thinking of guarding the Encode() calls with an error.IsEmpty() check but that's a good idea.

@bnoordhuis Done, PTAL!

@bnoordhuis Mind taking another look?

Mildly unrelated change.

The previous errors were incorrect here, as the code only failed in situations where strings exceeded size limits or an OOM situation was encountered, not for invalid encodings (which aren’t even detected explicitly). Unfortunately, these situations are hard to test for. PR-URL: nodejs#14579 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

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

This should be ready.

@addaleax ... fyi https://ci.nodejs.org/job/node-test-commit-aix/8108/nodes=aix61-ppc64/console

Can you check to see if this failure is related to the change?

Ugh … a lot of the Linux failures are the same, so, probably? test-commit-linux on master for comparison: https://ci.nodejs.org/job/node-test-commit-linux/12023/

Sigh. Yes, also a problem on master, so I’ll take a look.

I'm digging in also. :-) This wasn't a problem earlier today so it has to be one of the PRs landed today.... and I can't get to Jenkins... sigh. Will keep trying

@jasnell I think it’s only 32-bit systems (?) My guess would be

node/src/node_perf.cc

Lines 318 to 327 in c6da5c8

    
           #define SET_STATE_TYPEDARRAY(name, type, field)                         \ 
        
             target->Set(context,                                                  \ 
        
                         FIXED_ONE_BYTE_STRING(isolate, (name)),                   \ 
        
                         type::New(state_ab,                                       \ 
        
                                   offsetof(performance_state, field),             \ 
        
                                   arraysize(state->field)))                       \ 
        
                                             .FromJust() 
        
             SET_STATE_TYPEDARRAY("observerCounts", v8::Uint32Array, observers); 
        
             SET_STATE_TYPEDARRAY("milestones", v8::Float64Array, milestones); 
        
           #undef SET_STATE_TYPEDARRAY

I realize that’s kind of my code because it mirrors the HTTP2 one, so … uuh. 😄

Heh, okay, I think I got this.

Um.. that's confusing then :-/ ... the CI was good for that PR even on 32 bit system.

What do you have?

Landed in 784c6d4

(for the record, the above conversation was finished in #14996)

The previous errors were incorrect here, as the code only failed in situations where strings exceeded size limits or an OOM situation was encountered, not for invalid encodings (which aren’t even detected explicitly). Unfortunately, these situations are hard to test for. PR-URL: #14579 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

The previous errors were incorrect here, as the code only failed in situations where strings exceeded size limits or an OOM situation was encountered, not for invalid encodings (which aren’t even detected explicitly). Unfortunately, these situations are hard to test for. PR-URL: nodejs/node#14579 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. fs Issues and PRs related to the fs subsystem / file system. os Issues and PRs related to the os subsystem. labels Aug 1, 2017

bnoordhuis requested changes Aug 2, 2017

View reviewed changes

addaleax force-pushed the src-error-usage branch 3 times, most recently from 4951e75 to 84a16b0 Compare August 7, 2017 12:48

jasnell approved these changes Aug 8, 2017

View reviewed changes

bnoordhuis approved these changes Aug 19, 2017

View reviewed changes

addaleax force-pushed the src-error-usage branch from 84a16b0 to 116447a Compare August 23, 2017 23:24

addaleax closed this Aug 24, 2017

addaleax deleted the src-error-usage branch August 24, 2017 19:09

jasnell mentioned this pull request Oct 30, 2017

Proposal: 2017-10-31, Version 9.0.0 (Current) #15135

Merged

src: use proper errors as coming from StringBytes by addaleax · Pull Request #14579 · nodejs/node · GitHub

Uh oh!

Uh oh!

src: use proper errors as coming from StringBytes - #14579

src: use proper errors as coming from StringBytes#14579addaleax wants to merge 1 commit intonodejs:masternodejs/node:masterfrom addaleax:src-error-usageaddaleax/node:src-error-usageCopy head branch name to clipboard

Conversation

addaleax commented Aug 1, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Aug 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Aug 23, 2017

Uh oh!

jasnell commented Aug 23, 2017

Uh oh!

addaleax commented Aug 23, 2017

Uh oh!

addaleax commented Aug 23, 2017

Uh oh!

jasnell commented Aug 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Aug 24, 2017

Uh oh!

addaleax commented Aug 24, 2017

Uh oh!

jasnell commented Aug 24, 2017

Uh oh!

addaleax commented Aug 24, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

src: use proper errors as coming from StringBytes#14579
addaleax wants to merge 1 commit into
nodejs:masterfrom
addaleax:src-error-usage

jasnell commented Aug 24, 2017 •
edited
Loading