Migrate to "node2" debug engine #390

vladimir-kotikov · 2017-02-01T14:45:11Z

Use "node2" debugging engine (https://github.com/Microsoft/vscode-node-debug2) rather than "node" for debugging react-native code. As mentioned in #377 previous approach to debugging will be deprecated in Node 8.

This should resolve a lot of debugging issues we've seen previously, such as #367 and #356

- target es6 syntax as Chrome debug client seems to be targeting es6 - check for app worker's existence before disposing - make sure packager port is known when handling attach request

- bail out properly - use real path of node2 debugger package

- use raw NodeDebugSession to report error - improve logging in app worker

vladimir-kotikov · 2017-02-01T14:52:28Z

@MSLaguana, @roblourens I would really appreciate any feedback on this

MSLaguana · 2017-02-01T17:52:11Z

src/debugger/forkedAppWorker.ts

+// tslint:disable-next-line:align
+const WORKER_BOOTSTRAP = `
+// Hacks to avoid accessing unitialized variables
+var onmessage=null, self=global;


Should you also reference importScripts here?

MSLaguana · 2017-02-01T17:54:56Z

src/debugger/forkedAppWorker.ts

+    process.send(message);
+};
+importScripts = function(scriptUrl){
+    var scriptCode = require("fs").readFileSync(scriptUrl, "utf8");


I'm cautious about these requires here. I know that react-native redefines global require, so I'm not sure whether that can impact this if importScripts is called more than once. Might be worth using an IIFE to take a reference to the original require function initially and use that when importScripts is called? Or even just cache both fs and vm immediately and use those in importScripts

Great idea! I totally missed that fact with require override by react

MSLaguana · 2017-02-01T18:11:57Z

src/debugger/forkedAppWorker.ts

+        // Before sending messages, make sure that the worker is loaded
+        this.workerLoaded.promise
+        .then(() => {
+            if (rnMessage.method !== "executeApplicationScript") return Q.resolve(rnMessage);


Shouldn't other messages be passed on to the other process to be run in there?

Oh, I see that is handled in the done coming up next.

MSLaguana · 2017-02-01T18:20:15Z

src/debugger/forkedAppWorker.ts

+importScripts = function(scriptUrl){
+    var scriptCode = require("fs").readFileSync(scriptUrl, "utf8");
+    require("vm").runInThisContext(scriptCode, { filename: scriptUrl });
+};`;


Something else that I would recommend is adding a global __debug__ object with a require function which caches the node require. This matches what we currently expose in the original sandboxing method, and allows things such as realm to provide shims for native functionality when necessary.

roblourens · 2017-02-01T18:43:44Z

LGTM. You may want to follow this issue - microsoft/vscode#19650 - as we're talking about how the two debug adapters will interact in the future. It probably won't mean anything for you, at least for February. There's a chance they could end up merged into a single extension or something, but probably not.

MSLaguana · 2017-02-01T21:41:21Z

On the whole this looks pretty good to me. Am I correct that the SingleLifetimeAppWorker is no longer used at all, and is replaced by the forking app worker?

vladimir-kotikov · 2017-02-02T12:08:32Z

Thanks for review, guys!

Am I correct that the SingleLifetimeAppWorker is no longer used at all, and is replaced by the forking app worker?

That's correct. I initially thought that we might want to keep them both, just in case if we plan to support old "node" debugger, but this doesn't seem to be the case. Am i right? Do you want me to drop that old worker code?

MSLaguana · 2017-02-02T17:58:33Z

src/debugger/appWorker.ts

+        function killWorker(worker: IDebuggeeWorker) {
+            if (!worker) return;
+            worker.stop();
+            worker = null;


This won't modify this.worker which I believe was your intent.

MSLaguana · 2017-02-02T18:00:06Z

Looks like one of our tests is failing now, but it seems to be specifically to do with the single lifetime worker which isn't as relevant any more. Could you please look into that and update or remove the test as appropriate?

MSLaguana · 2017-02-02T18:26:14Z

Also to answer your question about the single lifetime worker: I think it would be best to only have the one way of doing things, so I am inclined to remove the single lifetime worker and just focus on the forked worker.

Worker script needs to be downloaded from packager and patched only once. However it was downloaded every time when new SingleLifetime worker was starting. Moving the download logic to MultipleLifetime worker would improve reload time. Also extract worker downloading logic for better testability

vladimir-kotikov · 2017-02-08T12:21:48Z

@MSLaguana, addressed your comments and updated tests to actually test current worker implementation. Please take a look once more

MSLaguana · 2017-02-08T20:48:17Z

src/test/debugger/appWorker.test.ts

            });

            test("should be able to import scripts", function() {
-                const scriptImportPath = "testScript.js";
+                // NOTE: we're not able to mock out


Looks like this comment is unfinished?

Yeah, there should be a reasoning why we have to use real script file to test importScripts. I'll update.

MSLaguana · 2017-02-08T20:56:27Z

src/test/debugger/appWorker.test.ts

                    // We have not yet finished importing the script, we should not have posted a response yet
                    assert(postReplyFunction.notCalled, "postReplyFuncton called before scripts imported");
-                    scriptImportDeferred.resolve(void 0);
-                    return Q.delay(1);
+                    return Q.delay(100);


What's the reasoning behind increasing this limit here? Before we used 1 just to make sure that it was asynchronous. Is the delay of 100ms actually required for processing to happen on time, are there potential race conditions here?

I think that previously the timeout was required just to delay dispatching the current promise chain and let scriptImportDeferred chain to be dispatched first.

In this implementation the spawned process is a black box and completely async for us so we can't reliably determine when the script has been imported and executed, so the only way to test this IMO is to give some reasonable amount of time to worker to complete script's import before checking if it has been done.

As for race conditions - not sure where are they might happen. Every test spawns it's own worker, so they are isolated from each other.

MSLaguana · 2017-02-08T20:57:07Z

Just a couple of questions, looking good.

MSLaguana · 2017-02-09T17:18:28Z

Looks great, thanks @vladimir-kotikov!

Vladimir Kotikov added 6 commits January 31, 2017 16:26

Reimplement debug adapter based on node2 adapter

e45838c

Fix number of issues appeared in prod

f920e58

- target es6 syntax as Chrome debug client seems to be targeting es6 - check for app worker's existence before disposing - make sure packager port is known when handling attach request

Add more logging to diagnose debug issues in prod

e38efae

Launch debuggee in Node rather than in Electron

a593f33

Address FIXMEs

8049d42

- bail out properly - use real path of node2 debugger package

Address TODOs:

a8b90ac

- use raw NodeDebugSession to report error - improve logging in app worker

msftclas added the cla-not-required label Feb 1, 2017

MSLaguana reviewed Feb 1, 2017

View reviewed changes

Vladimir Kotikov added 2 commits February 2, 2017 14:59

Address review notes

4b86d59

Resolve issues with reloading app on Android

e7b314e

MSLaguana reviewed Feb 2, 2017

View reviewed changes

Vladimir Kotikov added 5 commits February 3, 2017 18:21

Remove SingleLifetime worker, update tests

b05b508

Dispose worker properly when reloading

9174feb

Update app worker tests

039239d

Bump Node version on Travis

e90226e

MSLaguana reviewed Feb 8, 2017

View reviewed changes

CR: Update unfinished comment

553b717

MSLaguana merged commit dd977f5 into microsoft:master Feb 9, 2017

vladimir-kotikov deleted the node-debug2-v3 branch June 26, 2017 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Migrate to "node2" debug engine #390

Migrate to "node2" debug engine #390

vladimir-kotikov commented Feb 1, 2017 •

edited

Loading

vladimir-kotikov commented Feb 1, 2017

MSLaguana Feb 1, 2017

MSLaguana Feb 1, 2017

vladimir-kotikov Feb 1, 2017

MSLaguana Feb 1, 2017

MSLaguana Feb 1, 2017

MSLaguana Feb 1, 2017

roblourens commented Feb 1, 2017

MSLaguana commented Feb 1, 2017

vladimir-kotikov commented Feb 2, 2017 •

edited

Loading

MSLaguana Feb 2, 2017

MSLaguana commented Feb 2, 2017

MSLaguana commented Feb 2, 2017

vladimir-kotikov commented Feb 8, 2017

MSLaguana Feb 8, 2017

vladimir-kotikov Feb 9, 2017 •

edited

Loading

MSLaguana Feb 8, 2017

vladimir-kotikov Feb 9, 2017

MSLaguana commented Feb 8, 2017

MSLaguana commented Feb 9, 2017

Migrate to "node2" debug engine #390

Migrate to "node2" debug engine #390

Conversation

vladimir-kotikov commented Feb 1, 2017 • edited Loading

vladimir-kotikov commented Feb 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

roblourens commented Feb 1, 2017

MSLaguana commented Feb 1, 2017

vladimir-kotikov commented Feb 2, 2017 • edited Loading

Choose a reason for hiding this comment

MSLaguana commented Feb 2, 2017

MSLaguana commented Feb 2, 2017

vladimir-kotikov commented Feb 8, 2017

Choose a reason for hiding this comment

vladimir-kotikov Feb 9, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSLaguana commented Feb 8, 2017

MSLaguana commented Feb 9, 2017

vladimir-kotikov commented Feb 1, 2017 •

edited

Loading

vladimir-kotikov commented Feb 2, 2017 •

edited

Loading

vladimir-kotikov Feb 9, 2017 •

edited

Loading