debugger: remove obsolete setTimeout#7154

Trott · 2016-06-04T20:16:03Z

Checklist

tests and code linting passes
the commit message follows commit guidelines

Affected core subsystem(s)

debugger

Description of change

Remove obsolete setTimeout() introduced in 3148f14. The fix for the
problem is in b266074. (For the record, I mostly don't know what I'm
talking about here but am summarizing from an IRC #node-dev conversation
with @indutny on 04-Jun-2016.)

R=@indutny

indutny · 2016-06-04T20:18:34Z

LGTM if CI is green

Trott · 2016-06-04T20:39:40Z

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

Trott · 2016-06-04T21:31:34Z

Alas, it fails on Windows. https://ci.nodejs.org/job/node-test-binary-windows/2425/

not ok 49 parallel/test-debugger-pid # c:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vcbt2015\label\win10\Release\node.exe debug -p 655555 # # assert.js:90 # throw new assert.AssertionError({# ^ # AssertionError: '(node:4000) There was an internal error in Node\'s debugger. Please report this bug.' === '_debugger.js:1670\r' # at ChildProcess.<anonymous> (C:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vcbt2015\label\win10\test\parallel\test-debugger-pid.js:49:10) # at emitOne (events.js:96:13) # at ChildProcess.emit (events.js:188:7) # at C:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vcbt2015\label\win10\test\parallel\test-debugger-pid.js:18:16 # at Array.forEach (native) # at Socket.onData (C:\workspace\node-test-binary-windows\RUN_SUBSET\2\VS_VERSION\vcbt2015\label\win10\test\parallel\test-debugger-pid.js:17:8) # at emitOne (events.js:96:13) # at Socket.emit (events.js:188:7) # at readableAddChunk (_stream_readable.js:172:18) # at Socket.Readable.push (_stream_readable.js:130:10) --- duration_ms: 0.308

Fishrock123 · 2016-06-04T22:06:44Z

@Trott what happens if you use setImmediate()?

Trott · 2016-06-05T04:47:12Z

CI with change to setImmediate() instead of full timer elimination: https://ci.nodejs.org/job/node-test-pull-request/2924/

Trott · 2016-06-05T05:24:55Z

setImmediate() seems to be just fine on Windows. 🎉

Trott · 2016-06-05T05:26:05Z

Re-running CI again because all the ARM stuff blew up, wow: https://ci.nodejs.org/job/node-test-pull-request/2925/

cjihrig · 2016-06-05T13:32:23Z

LGTM. The CI blew up again though.

Fishrock123 · 2016-06-05T13:52:08Z

lib/_debugger.js

self.run(self.resume)?

Also can you drop self for this now?

Also can you drop self for this now?
Oh, right, arrow functions, lexical binding...

self.run(self.resume)?
test/parallel/test-debugger-util-regression.js fails if self.resume/this.resume is not wrapped in a function. (I did not look to see why...)

Probably because of this not being set properly.

Trott · 2016-06-06T04:26:28Z

Post-nit-fix CI: https://ci.nodejs.org/job/node-test-pull-request/2932/

cjihrig · 2016-06-06T12:54:01Z

lib/_debugger.js

You're no longer calling resume()?

Heh, looks like we don't have a test that covers that. Guess I'll be adding one.

It seems that .resume() might be unnecessary:
.run() calls either .error() or trySpawn()
.error() will call .resume()
trySpawn() will call .resume() in a ready event listener on client

@indutny

Remove obsolete `setTimeout()` introduced in 3148f14. The fix for the problem is in b266074. (For the record, I mostly don't know what I'm talking about here but am summarizing from an IRC #node-dev conversation with @indutny on 04-Jun-2016.)

Trott · 2016-06-06T22:37:03Z

rebased, removed .resume() that was accidentally discovered to be unnecessary, force pushed...

@indutny @cjihrig Can you confirm that this still looks good to you (assuming CI doesn't have any surprises)?

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

cjihrig · 2016-06-07T00:51:30Z

LGTM, but I'll defer to @indutny, who appears to have written that code back in 2011 (3148f14).

indutny · 2016-06-07T01:11:42Z

Still LGTM

@indutny

Remove obsolete `setTimeout()` introduced in 3148f14. The fix for the problem is in b266074. (For the record, I mostly don't know what I'm talking about here but am summarizing from an IRC #node-dev conversation with @indutny on 04-Jun-2016.) PR-URL: nodejs#7154 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Trott · 2016-06-07T20:33:04Z

Landed in 671cffa

@indutny

Remove obsolete `setTimeout()` introduced in 3148f14. The fix for the problem is in b266074. (For the record, I mostly don't know what I'm talking about here but am summarizing from an IRC #node-dev conversation with @indutny on 04-Jun-2016.) PR-URL: #7154 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

MylesBorins · 2016-07-11T22:54:57Z

@Trott lts?

Trott · 2016-07-11T22:56:56Z

@thealphanerd I think so, but maybe check with @indutny ?

indutny · 2016-07-11T22:58:59Z

If CI is good - LGTM

@indutny

Remove obsolete `setTimeout()` introduced in 3148f14. The fix for the problem is in b266074. (For the record, I mostly don't know what I'm talking about here but am summarizing from an IRC #node-dev conversation with @indutny on 04-Jun-2016.) PR-URL: #7154 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

@indutny

Remove obsolete `setTimeout()` introduced in 3148f14. The fix for the problem is in b266074. (For the record, I mostly don't know what I'm talking about here but am summarizing from an IRC #node-dev conversation with @indutny on 04-Jun-2016.) PR-URL: #7154 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

@indutny

Remove obsolete `setTimeout()` introduced in 3148f14. The fix for the problem is in b266074. (For the record, I mostly don't know what I'm talking about here but am summarizing from an IRC #node-dev conversation with @indutny on 04-Jun-2016.) PR-URL: #7154 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

@indutny

Remove obsolete `setTimeout()` introduced in 3148f14. The fix for the problem is in b266074. (For the record, I mostly don't know what I'm talking about here but am summarizing from an IRC #node-dev conversation with @indutny on 04-Jun-2016.) PR-URL: #7154 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

@indutny

Remove obsolete `setTimeout()` introduced in 3148f14. The fix for the problem is in b266074. (For the record, I mostly don't know what I'm talking about here but am summarizing from an IRC #node-dev conversation with @indutny on 04-Jun-2016.) PR-URL: #7154 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Trott added the debugger label Jun 4, 2016

Fishrock123 reviewed Jun 5, 2016
View reviewed changes

Trott force-pushed the fixhack branch from 25fc766 to 3683e0cCompare June 6, 2016 04:24

cjihrig reviewed Jun 6, 2016
View reviewed changes

Trott added 4 commits June 6, 2016 14:11

debugger: remove obsolete setTimeout
909009f
Remove obsolete `setTimeout()` introduced in 3148f14. The fix for the problem is in b266074. (For the record, I mostly don't know what I'm talking about here but am summarizing from an IRC #node-dev conversation with @indutny on 04-Jun-2016.)

squash: try setImmediate
a0d60c4

squash: self -> this
3406e1b

squash: remove unneeded resume()
2bda6c1

Trott force-pushed the fixhack branch from 3683e0c to 2bda6c1Compare June 6, 2016 22:35

Trott closed this Jun 7, 2016

evanlucas mentioned this pull request Jun 16, 2016
v6.2.2 proposal #7323
Merged

MylesBorins mentioned this pull request Jul 11, 2016
Audit commits not found on v4.x-staging #7661
Closed

MylesBorins added the lts-watch-v4.x label Jul 11, 2016

MylesBorins added land-on-v4.x and removed lts-watch-v4.x labels Jul 11, 2016

MylesBorins mentioned this pull request Jul 12, 2016
v4.5.0 proposal #7688
Merged

Trott deleted the fixhack branch January 13, 2022 22:43

Uh oh!

debugger: remove obsolete setTimeout#7154

debugger: remove obsolete setTimeout #7154

Uh oh!

Conversation

Trott commented Jun 4, 2016

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

indutny commented Jun 4, 2016

Uh oh!

Trott commented Jun 4, 2016

Uh oh!

Trott commented Jun 4, 2016

Uh oh!

Fishrock123 commented Jun 4, 2016

Uh oh!

Trott commented Jun 5, 2016

Uh oh!

Trott commented Jun 5, 2016

Uh oh!

Trott commented Jun 5, 2016

Uh oh!

cjihrig commented Jun 5, 2016

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!

Trott commented Jun 6, 2016

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!

Trott commented Jun 6, 2016

Uh oh!

cjihrig commented Jun 7, 2016

Uh oh!

indutny commented Jun 7, 2016

Uh oh!

Trott commented Jun 7, 2016

Uh oh!

MylesBorins commented Jul 11, 2016

Uh oh!

Trott commented Jul 11, 2016

Uh oh!

indutny commented Jul 11, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants