fix(xsnap-worker): require explicit $XSBUG_HOST for debugging

[michaelfig]

I was dismayed to find xsnap-worker spinning for a minute before becoming responsive. As it turns out, my machine was running a different service on localhost:5002, and the worker was in a loop trying to connect to it, expecting it was xsbug.

To fix xsnap-worker, I reset the default xsbug port to -1, and short-circuit out of the debug connection in that case. If you need to debug the worker, just explicitly use XSBUG_HOST=localhost:5002 ./xsnap-worker ....

[phoddie]

The makefile for xsnap builds both debug and release versions of the tool. The release version has xsbug support compiled out (see #ifdef mxDebug). Rather than invent a new mechanism to control whether JavaScript debugging is enabled, could it be enough to run the release build?

[dckc]

I thought we resolved the debug build mode design in Agoric/agoric-sdk#2578 (under the original title of XS / xsnap build mode: debug? production? mix?) but I don't see a clear resolution.

In any case, let's do get clear on what the issue is. It sounds like a weird performance issue, but it's not entirely clear to me how to reproduce it. What are the steps, @michaelfig ?

[michaelfig]

The makefile for xsnap builds both debug and release versions of the tool. The release version has xsbug support compiled out (see #ifdef mxDebug). Rather than invent a new mechanism to control whether JavaScript debugging is enabled, could it be enough to run the release build?

Oh, is that the intent? The release version of xsnap-worker is currently being compiled with -DmxDebug=1. See both https://github.com/agoric-labs/xsnap-pub/blob/master/xsnap/makefiles/lin/xsnap-worker.mk#L32 and https://github.com/agoric-labs/xsnap-pub/blob/master/xsnap/makefiles/mac/xsnap-worker.mk#L37

I'll put together a PR that implements the behaviour you describe for xsnap-worker, instead.

[michaelfig]

In any case, let's do get clear on what the issue is. It sounds like a weird performance issue, but it's not entirely clear to me how to reproduce it. What are the steps, @michaelfig ?

nc -l localhost 5002 </dev/null &

then switch to another terminal:

xsnap-native/xsnap/build/bin/mac/release/xsnap-worker

Notice that the first terminal looks like:

<xsbug><login name="xsnap" value="XS"/></xsbug>

and the xsnap-worker spins even after you ^C the nc window.

output from top shows 99% CPU:

61715  xsnap-worker 99.3 00:07.27 1/1    0    14    41M    0B     0B     61715 2543  running  *0[1]            0.00000

[dckc]

... The release version of xsnap-worker is currently being compiled with -DmxDebug=1.

Yes, as I say, this is the result of considerable design discussion under Agoric/agoric-sdk#2578

p.s. found the resolution: Agoric/agoric-sdk#3776 , which depends on #1 here.

I'll put together a PR that implements the behaviour you describe for xsnap-worker, instead.

I'm still not clear on what new information merits revisiting the earlier design decision. So I guess I don't mind if you make such a PR, but let's not land it until we are clear on the issue and get buy-in from all the stakeholders in the earlier decision.

[dckc]

Note the discussion in Agoric/agoric-sdk#3776 includes:

this probably enables the debugger network connection in release / production mode.

So this behavior when something is running on port 5002 is by design, at least to some extent. We weren't aware of this potential performance impact, though. That's new information.

[michaelfig]

this probably enables the debugger network connection in release / production mode.

So this behavior when something is running on port 5002 is by design, at least to some extent. We weren't aware of this potential performance impact, though. That's new information.

Okay, then I stand by this PR. I see we want mxDebug mode for its diagnostic information, but xsnap-worker really shouldn't reach out on the network without explicit configuration . Indeed, once we start using seccomp to lock things down, this network attempt would kill the process.

Semi-related to this, I'll put up a PR that at least stops the spinning. But I would much rather prevent the connection entirely under release mode.

[phoddie]

If the sole goal is to require XSBUG_HOST to be defined in the environment to allow the xsbug connection, maybe it is simpler in fxConnect to change these lines...

	else {
		strcpy(name, "localhost");
		port = 5002;
	}

...to this?

	else {
		return;
	}

[phoddie]

Thanks for the quick change. That looks reasonable to me (from an XS perspective; I cannot speak to the comments raised by @dckc regarding Agoric's goals here).

[dckc]

This looks consistent with earlier decisions.

I wonder if XSBUG_HOST merits mention in https://github.com/Agoric/agoric-sdk/blob/master/docs/env.md

I see XSNAP_DEBUG from SwingSet/src/controller.js is not documented there. Ideally both would be documented there, but this PR is OK independent of that.