Cannot step over a fiber yield when debugging server with Meteor 1.6

[mattmccutchen]

When I'm debugging the server of a Meteor 1.6 application using the Node.js built-in debugger, when I try to step over a function call that yields the current fiber, the server goes on running and does not stop on the next line of code. Using Meteor 1.5.2.2 and node-inspector, the server properly stops on the next line. Maybe it's possible to use node-inspector with Meteor 1.6, but this doesn't look promising. So this looks like a significant loss of debugging functionality for server code that (for example) writes to Mongo collections. This may be enough grounds for me to hold off on upgrading to Meteor 1.6.

I imagine the underlying incompatibility between fibers and the Node.js built-in debugger is nothing Meteor-specific, so I was surprised not to find any discussion of the problem in a few web searches.

As requested by the bug template, I'm using the 64-bit Linux version of Meteor; I doubt it matters.

Reproduction:

1. Run:

git clone https://github.com/mattmccutchen/meteor-debug-yield-repro
cd meteor-debug-yield-repro
meteor npm install    # https://github.com/meteor/meteor/pull/8165 ...
meteor debug

2. Open http://localhost:8080/?port=5858 in Google Chrome. You should be on the second debugger statement (there seems to be a problem with stopping early in server startup).
3. Press F10 repeatedly. The debugger should eventually step to console.log("after"); and stop there.
4. Exit the server with Ctrl-C.
5. Run: meteor --release=1.6 --inspect
6. Open the Google Chrome-based debugger (about:inspect, "Open dedicated DevTools for Node"). You should be on the first debugger statement.
7. Press "Resume". You should now be on the second debugger statement.
8. Press F10 repeatedly. When you are on f.wait(); and press F10, the console.log("after"); executes without the debugger first stopping on that line.

[benjamn]

Once you're paused on that debugger statement, can you set a breakpoint on the line after the yielding function call, and will the debugger stop there?

[benjamn]

In my own experience, the old node-inspector was so slow and broken in so many ways, it's surprising that someone would want to switch back because of this annoyance. I mean, I hope we can fix it, but the other benefits of the Node 8 inspector far outweigh this problem. I hope you can give the new inspector enough of a try to appreciate its superiority.

[mattmccutchen]

Once you're paused on that debugger statement, can you set a breakpoint on the line after the yielding function call, and will the debugger stop there?

Yes, that's a potential workaround. It does mean that when stepping, I have to remember which functions might yield, and if I forget one and the server runs past the code I care about, I have to start the test over. Maybe this won't be so bad once I get used to it.

What do you think about referencing this issue from the item in History.md about the changes to debugging? It took me an hour and a half to track down this problem and I would want to try to save other users from wasting time, although maybe the fact that the problem wasn't found earlier in the testing of Meteor 1.6 suggests that users rarely try to step over a Mongo collection write on the server; I expected that would be common during debugging.

[veered]

I think this is a very large limitation. It means that you can't really step through a function (w/o setting additional breakpoints anyways) and that many commands run in the debug console won't work properly.

Is there a way of intercepting commands sent from the Chrome debugger to the Node server?

[benjamn]

After multiple days of debugging, I have tracked this problem down to a pair of methods that were never fully implemented in V8:

char* Debug::ArchiveDebug(char* storage) {
  // Simply reset state. Don't archive anything.
  ThreadInit();
  return storage + ArchiveSpacePerThread();
}

char* Debug::RestoreDebug(char* storage) {
  // Simply reset state. Don't restore anything.
  ThreadInit();
  return storage + ArchiveSpacePerThread();
}

These methods are important because they are called whenever a new thread takes control of the V8 Isolate object, and Fiber switching looks like thread switching from the perspective of V8 (even though no actual threads are spawned).

The good news is that the fibers library isn't doing anything wrong here, besides not properly locking the V8 Isolate object when switching Fibers, causing some assertion failures with debug builds of Node.js (and I have a PR for that).

I was deeply worried for a few days that we would have to find an alternative to fibers, or encourage the community to stop using them. As long as fibers behave like threads as far as V8 is concerned, and V8 continues to tolerate multiple threads (which it will), I'm confident the idea of fibers/coroutines will remain viable. Thanks again to @laverdet for keeping https://github.com/laverdet/node-fibers up-to-date across so many major Node.js versions.

My initial post to the v8-users list: https://groups.google.com/forum/#!topic/v8-users/_Qf2rwljRk8
Bug: https://bugs.chromium.org/p/v8/issues/detail?id=7230
PR: https://chromium-review.googlesource.com/c/v8/v8/+/833260

Until that PR is merged into V8 (days, weeks?), and Node.js updates to that new version of V8 (months?), and Meteor updates to that version of Node.js (following the LTS schedule), we will have to back-port this patch manually, and ship Meteor with a patched version of Node.js. That's not ideal, but it means we can fix this problem in Meteor 1.6.1, rather than waiting… years?

[veered]

@benjamn You are amazing :)

[benjamn]

Since this has been diagnosed as an upstream-upstream bug, and fixing it in Meteor 1.6.1 would require building a custom version of Node and V8, I think it may be safer to wait until Meteor 1.6.2, just because the jump from using an official Node release to one we compiled ourselves is non-trivial, and I'd like to get Meteor 1.6.1 out sooner rather than later. (cc @abernix @hwillson)

[hwillson]

I completely agree @benjamn - the 1.6.1 commit list is getting pretty big and there are some great things ready to go. Holding off on including a custom Node release until 1.6.2 sounds good to me.

[Gitii]

I think, I am experiencing similar behaviour with the latest webstorm.
Stepping over (F10) seems to "detach" the debugger.

[veered]

@benjamn It seems that https://chromium-review.googlesource.com/c/v8/v8/+/833260 never got merged; any chance you could take another look and try to get it merged?

[benjamn]

Hopefully this week! https://chromium-review.googlesource.com/c/v8/v8/+/833260#message-0175d625e67e0c0c63d58595dcecd49aeb8b67d4

[veered]

Awesome, thanks!

[benjamn]

Ok, I have written a much better test for my V8 changes, which revealed some flaws in my previous implementation of Debug::{Archive,Restore}Debug, but I think I have fixed those problems, and now I'm waiting for review from the V8 folks: https://chromium-review.googlesource.com/c/v8/v8/+/833260

[veered]

@benjamn woooooooooooooooooooooooooooooooo thanks!

The Chrome team recently released ndb (https://github.com/GoogleChromeLabs/ndb). Has some pretty neat features. Might make the debugging experience even better.

Hmm... have you ever tried using the devtools for working with the build tool?

[klaussner]

Hmm... have you ever tried using the devtools for working with the build tool?

@veered I always use TOOL_NODE_FLAGS="--inspect" meteor to debug the build tool and it works great! 👌

[benjamn]

My CR was just merged into V8 master! v8/v8@a8f6869

[veered]

That's awesome! The debug tools will now be way way more useful

[benjamn]

My change has now been merged into the master branch of Node: nodejs/node@bb35752

It's also slated to be included in Node 10.10.0: nodejs/node#22716

I'm still hoping it can be included in Node 8.12.0, pending this PR: nodejs/node#22714

[nicooprat]

Chrome DevTools team recently launched support for NDB: https://developers.google.com/web/updates/2018/08/devtools#ndb

Any chance to use it with Meteor? Tried to launch Meteor via ndb but it won't start (no error, nothing happens). Did I miss something? Do we need to wait for you PR to be merged?

Thanks.

Related https://forums.meteor.com/t/meteor-and-new-ndb-for-debugging/44818

[benjamn]

The v8.x-staging PR was just merged (nodejs/node#22714), so this change should become available in Node 8.13.0!

@nicooprat I would imagine you can still run meteor --inspect-brk and connect to the process using ndb after it launches. Seems like a loss of functionality if you have to use ndb instead of node to launch the process.

[Discordius]

Regarding the ndb support: I can't seem to figure out a way to make ndb run. It does look like you need to use ndb instead of the normal node call. Any advice on making it run would be appreciated.

[benjamn]

I'm thrilled to report this bug is finally fixed after running meteor update --release 1.8.1-beta.6 to update to Node 8.13.0, the first Node 8 version to include v8/v8@a8f6869 via nodejs/node#22714.

I can't think of any other bugs I've encountered in my career that consumed this much time (though maybe I've blocked them out), but I guess that's what happens when you're dealing with subtle C++ multi-threading bugs in an upstream-upstream project.

[benjamn]

In order to reflect what's been fixed in the Meteor 1.8.1 milestone so far, I'm going to close this issue now, though of course the fix is not in any official Meteor release yet.