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.

Sign up for GitHub

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

Jump to bottom

lib: clean up usage of threw #10534

Closed
wants to merge 1 commit into from
Closed

lib: clean up usage of threw #10534

wants to merge 1 commit into from

Conversation

JacksonTian
Copy link
Contributor

@JacksonTian JacksonTian commented Dec 30, 2016 •
edited
Loading

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

bootstrap_node, module, timers

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. module Issues and PRs related to the module subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout. lts-watch-v6.x labels Dec 30, 2016
lpinca
lpinca reviewed Dec 30, 2016
View reviewed changes
lib/timers.js Outdated
} finally {
if (!threw) return;

} catch (ex) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The exception is no longer thrown now, is this wanted?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, looks like @JacksonTian forgot to rethrow the ex

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much. fixed it.

@jasnell
Copy link
Member

jasnell commented Dec 30, 2016

hmm... @Fishrock123 are you familiar with why threw was in here in the first place? And why this was being handled in a finally block rather than in a catch?

@Fishrock123
Copy link
Contributor

I think this is a performance optimization of some sort. This should be profiled. (e.g. --prof, not just microbenchmarks.)

@gibfahn gibfahn added the performance Issues and PRs related to the performance of Node.js. label Dec 31, 2016
@JacksonTian
Copy link
Contributor Author

I hold the change of module.js, there are test cases check the append excpection message.

Fishrock123
Fishrock123 previously requested changes Jan 3, 2017
View reviewed changes
Copy link
Contributor

@Fishrock123 Fishrock123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Requesting the aforementioned profiling)

@JacksonTian
Copy link
Contributor Author

@Fishrock123 How to profiling it? Hope to get some suggestions.

@jasnell
Copy link
Member

jasnell commented Mar 24, 2017

ping @Fishrock123 ...

any updates on this one?

@jasnell jasnell added the stalled Issues and PRs that are stalled. label Mar 24, 2017
@BridgeAR
Copy link
Member

BridgeAR commented Aug 26, 2017 •
edited
Loading

@JacksonTian this needs a rebase. And can you please comment when you did? There is no notification otherwise and I see that you rebased the last time you were asked to.

@JacksonTian
Copy link
Contributor Author

rebased.

BridgeAR
BridgeAR approved these changes Aug 26, 2017
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if CI is green

@BridgeAR
Copy link
Member

I benchmarked this recently and could not find any difference performance wise.
@Fishrock123 I am going to dismiss your review because of that. Please have another look.

@BridgeAR BridgeAR dismissed Fishrock123’s stale review August 26, 2017 22:34

No reaction and the performance should not be affected

@BridgeAR BridgeAR removed performance Issues and PRs related to the performance of Node.js. stalled Issues and PRs that are stalled. labels Aug 26, 2017
@BridgeAR
Copy link
Member

Landed in d56a82dfc3569332b1ad5a3249b8ca3405c37163

@BridgeAR BridgeAR closed this Aug 29, 2017
@BridgeAR
Copy link
Member

The test actually fails and I just force pushed to remove the commit again.

@BridgeAR BridgeAR reopened this Aug 29, 2017
@jasnell
Copy link
Member

jasnell commented Aug 29, 2017

It doesn't look like there were any CI runs for this so good call backing it out. Bonus points for it being within the 10 minute window ;-)

@BridgeAR
Copy link
Member

BridgeAR commented Aug 29, 2017 •
edited
Loading

I knew why I said

LGTM if CI is green

I somewhat had a bad feeling about it but I forgot to run it right before I pushed.

@JacksonTian
Copy link
Contributor Author

JacksonTian commented Aug 30, 2017 •
edited
Loading

Oh. Let's run CI again?

@JacksonTian
lib: clean up usage of threw
4d64611 
Use try/catch to instead of threw.
@JacksonTian
Copy link
Contributor Author

Sorry, My mistake. After rebased with master, the test message no more need update. Now I removed it.

@aqrln
Copy link
Contributor

aqrln commented Aug 30, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/9876/

@BridgeAR
Copy link
Member

Landed in 67d792a

@BridgeAR BridgeAR closed this Aug 30, 2017
BridgeAR pushed a commit that referenced this pull request Aug 30, 2017
@JacksonTian @BridgeAR
lib: clean up usage of threw
67d792a 
Use try/catch to instead of threw.

PR-URL: #10534
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
cjihrig pushed a commit to cjihrig/node that referenced this pull request Aug 31, 2017
@JacksonTian @cjihrig
lib: clean up usage of threw
73346f5 
Use try/catch to instead of threw.

PR-URL: nodejs#10534
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@JacksonTian JacksonTian deleted the threw branch September 2, 2017 07:58
addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 5, 2017
@JacksonTian @addaleax
lib: clean up usage of threw
298110e 
Use try/catch to instead of threw.

PR-URL: nodejs/node#10534
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
@JacksonTian @MylesBorins
lib: clean up usage of threw
1aa87bb 
Use try/catch to instead of threw.

PR-URL: #10534
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
@JacksonTian @MylesBorins
lib: clean up usage of threw
9129057 
Use try/catch to instead of threw.

PR-URL: #10534
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 20, 2017
@JacksonTian @MylesBorins
lib: clean up usage of threw
0a608ed 
Use try/catch to instead of threw.

PR-URL: #10534
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins
Copy link
Contributor

MylesBorins commented Sep 20, 2017 •
edited
Loading

backported to v6.x

Let me know if it should be backed out

@MylesBorins MylesBorins mentioned this pull request Sep 20, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Sep 26, 2017
@JacksonTian @MylesBorins
lib: clean up usage of threw
4e71782 
Use try/catch to instead of threw.

PR-URL: #10534
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca left review comments

@sam-github sam-github sam-github left review comments

@BridgeAR BridgeAR BridgeAR approved these changes

@cjihrig cjihrig cjihrig approved these changes

@jasnell jasnell jasnell approved these changes

@Fishrock123 Fishrock123 Fishrock123 left review comments

Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. module Issues and PRs related to the module subsystem. timers Issues and PRs related to the timers subsystem / setImmediate, setInterval, setTimeout.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.