debugger: remove obsolete setTimeout by Trott · Pull Request #7154 · nodejs/node

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 <fedor@indutny.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <fedor@indutny.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 <fedor@indutny.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

@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 <fedor@indutny.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

@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 <fedor@indutny.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

@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 <fedor@indutny.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

@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 <fedor@indutny.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com>

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 3683e0c Compare 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 2bda6c1 Compare 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!

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