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.

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

AVA debug --break is hardcoded to bind to loopback interface. #2406

Closed
DYefimov opened this issue Feb 19, 2020 · 10 comments · Fixed by #2413
Closed

AVA debug --break is hardcoded to bind to loopback interface. #2406

DYefimov opened this issue Feb 19, 2020 · 10 comments · Fixed by #2413
Labels

Comments

@DYefimov
Copy link
Contributor

Dockerized AVA in debug mode is difficult to reach due to hardcoded binding to loopback interface (127.0.0.1).

file: ava/lib/worker/subprocess.js:188
require('inspector').open(options.debug.port, '127.0.0.1', true);

Hacking it to 0.0.0.0 fixes the problem.
It would be nice if it defaults to loopback, and read, for example, 'options.debug.ip' as an override.

There are couple workarounds but none of them are easy/convenient.
socat -d tcp-listen:xxxx,reuseaddr,fork tcp:127.0.0.1:xxxx &
But meh...

@novemberborn novemberborn added enhancement new functionality help wanted and removed question labels Feb 20, 2020
@novemberborn
Copy link
Member

Fair enough.

We can add a "host" argument here:

ava/lib/cli.js

Line 134 in 324e45f

port: {

And then we can add it to the debug object:

ava/lib/cli.js

Line 148 in 324e45f

port: argv.port

IIRC it should then be available in the worker process:

require('inspector').open(options.debug.port, '127.0.0.1', true);

@romashko-bo-ma
Copy link

👋
any updates on this one?

@novemberborn
Copy link
Member

@romashko-bo-ma it's been all of three days since this issue was accepted… so clearly not!

Perhaps you'd be interested in picking this up?

@DYefimov
Copy link
Contributor Author

Fair enough.

We can add a "host" argument here:

ava/lib/cli.js

Line 134 in 324e45f

port: {

And then we can add it to the debug object:

ava/lib/cli.js

Line 148 in 324e45f

port: argv.port

IIRC it should then be available in the worker process:

require('inspector').open(options.debug.port, '127.0.0.1', true);

I can confirm, the above solution works perfectly, as far as I can see.

Unfortunately, I'm not confident enough to submit a PR for this one - I have 20 years of C++ professional experience and using JS just for fun projects, so don't wanna dive deeper into the environments I do not control completely. Sorry for the excuse :)

@novemberborn
Copy link
Member

No worries @DYefimov, though it sounds like you're really close to having a workable PR?

A good way to "force" things to happen in open source is to kick off a PR… I tend to respond to those first when I have time for AVA, instead of other coding tasks.

@DYefimov
Copy link
Contributor Author

What a diplomatic answer @novemberborn :)
Here is the PR then.

Actually, there is one more issue strongly connected to this one.
Namely how AVA handles debug mode at all.

Currently, AVA enters debug mode with the "debug flag cmd option", which requires a filename to "break on" supplied alongside.
This is completely counter-intuitive, and inconvenient.

Where "counter-intuitive" is: one expects to put a breakpoint in any file, and start debugging.
And "inconvenient" is: one should always remember to select the file to be debugged in an editor - so miss-clicks are common; or supply cmd "break on" option manually - which is a hassle.
Plus as a side effect: node-js "--inspect" - is something one expects to work by default - but this raises many weirdness.

All the above in concise form: AVA doesn't following default nodejs debug behavior, causing frustration, and configurational disaster.

Issues #2274 and #1505 have more info about this.

======

IMHO the root cause of all this nuisance is in node itself. It deceives developers into implementing hacks like AVA debug infrastructure.

First of all, node don't have multi-threading (until lately, I believe - i.e. worker_threads), so one in need have only option - to fork.

Fork is something that works on an operating system level.
And it never been simple.

Thus, to implement graceful (convenient, intuitive) debugging behavior, the calling process (nodejs in our case) should implement it by itself in a convenient way. And this is the main issue.

If we look closer into, for example, gdb - it has "follow-fork-mode" parameter, i.e. on fork - should we follow child or stay in a caller process?
If we outline an analogy to nodejs here:
gdb "inspector" (debug-client) has only one instance, and "debug boilerplate" (debug-server) decides where to follow on fork.
Nodejs doesn't behave the same way. Instead, it allows us to supply new parameters to a callee process. As a result - we can't connect to a child with the same debug port

I'm not a nodejs professional, so not completely sure in the above statement, but it seems it works this way.

If it is so, we have a couple of options here.

Option 1.
programmaticaly open only one port (like AVA's way)
result: we do drop default builtin "--inspect" functionality, i.e. implement our own debug infrastructure

Option 2.
programmaticaly open many ports (for all callee processes; maybe in some range)
result: we do not drop default builtin "--inspect" functionality
prerequisite: we need a mechanism to attach to all spawned children ("autoAttachChildProcesses" in vsvode? didn't test it, though)

Option 3.
Consider using worker_threads
result: we do not drop default builtin "--inspect" functionality
prerequisite: none

Conclusion.
Fork is a "heavy artillery" technology, which is best suited for stuff like process isolation, segfault (and other signals) control, and so on - read operating system level tasks. Where for concurrency worker_threads seems to be the most viable option here. If it is not possible/feasible to implement, option 2 will be less frustrating to work with for AVA newcomers. And meanwhile the big red box stating "Before debugging tests, carefully read this section" should be close to the top of the landing page.

P.S. This is not a "have to", but a mere "have in mind". Spent a day digging deeper into this topic, because it got me so frustrated while configuring. And I'm still unhappy with the results, as passing debug file path with config option introduces unnecessary level of complexity on a layer that is not tailored for such kind of tasks. Convolution => most likely bad design.

@novemberborn
Copy link
Member

All the above in concise form: AVA doesn't following default nodejs debug behavior, causing frustration, and configurational disaster.

What is this default behavior you're referring to?

The debug command is something we could ship, given our constraints. I've found it quite useful already. The ava command is not the node command.

@DYefimov
Copy link
Contributor Author

DYefimov commented Mar 2, 2020

It's definitely useful. But IMHO this is a 'hack' approach from the users perspective.

The default behavior would be:

  1. put a break point in any file of interest
  2. do not use AVAs debug flag, neither specify the filepath to debug
  3. instead supply node with --inspect
  4. start debugging anywhere in you project
  5. achieve successful break at the breakpoint

To implement this, the best solution would be - utilization of multithreading.
Of cause, I'm not aware of your plans, and maybe you want to use forking (process isolation), for some hypothetical multi-machine scenarios? testing farms? Or thinking about backward compatibility? Or something else? In this case fork (process isolation) is definitely needed.

But as far as I can tell, multithreading should solve this 'hack' approach, and simplify debugging experience.
Another (more ugly) option will be - autoAttachChildProcesses, and auto-increment port on forks. Didn't test it though yet.

@novemberborn
Copy link
Member

  • instead supply node with --inspect

But you don't call node. You use npx ava, or ava directly if you have it in your path.

  • start debugging anywhere in you project

How? You need to launch some other tool to attach a debugger first.

To implement this, the best solution would be - utilization of multithreading.

We want that regardless, actually.

I considered using the --inspect argument, but it got tricky when running multiple test files (how to wait for the right file to be ready to debug), and our argument parser behaves differently than Node.js itself.

@DYefimov
Copy link
Contributor Author

  • instead supply node with --inspect

But you don't call node. You use npx ava, or ava directly if you have it in your path.

Can't you just pass -- --inspect-brk to the underlying node process?

  • start debugging anywhere in you project

How? You need to launch some other tool to attach a debugger first.

So, presumably, passing --inspect-brk to the underlying process should work?

I considered using the --inspect argument, but it got tricky when running multiple test files (how to wait for the right file to be ready to debug), and our argument parser behaves differently than Node.js itself.

Dealt with it too, having messaging though pipes between forks on top of it, plus serialization and other blackjack - yeah, it becomes messy really fast. And inconvenient to debug. That's why I was wondering about multithreading, and if forking is really necessary in this case.

To implement this, the best solution would be - utilization of multithreading.

We want that regardless, actually.

Well, this answers most of the questions. IMHO this discussion is exhausted now.
And thanks for a patch and great tool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants