debugger,test: fix flaky debug logging on SmartOS

[Trott]

SmartOS sometimes munges output from process._rawDebug()
but Windows can't use console.error() in the debug agent.
So inject logic to chose the right thing based on platform.

Fixes: #2476

[Trott]

CI: https://ci.nodejs.org/job/node-test-pull-request/653/

[cjihrig]

===

[Trott]

I usually feel like I need a bath after typing == instead of ===. Not sure how that happened. Will change for sure.

[Trott]

Addressed @cjihrig suggestions, pushed a fixup commit, killed the prior CI and re-running.

New CI at: https://ci.nodejs.org/job/node-test-pull-request/654/

UPDATE: CI looks good. One went offline during the build failure and a single failure on Windows with a test that's been flaky for a while. Otherwise, all success.

[thefourtheye]

You can use common.isWindows

Edit: Nvm. This is not a test.

[jbergstroem]

is _rawDebug out of scope to fix?

[Trott]

@jbergstroem I suppose not, although I'm not even sure this is a bug in _rawDebug and not an OS bug. I don't have a SmartOS machine to mess around with.

[jbergstroem]

happy to give you access if you want to tinker around.

[Trott]

Cool, I may be in touch. (Going to focus on getting to sleep now.) BTW, it could also go the other way, I imagine, and be a "fix console.error() in this context for Windows" . If that happened, there would be no reason for _rawDebug() to exist. Of course, that might be an impossibility for all I know...

[Trott]

@jbergstroem My public keys are at https://github.com/trott.keys. What else would you need to give me access?

[jbergstroem]

@Trott ping me at bugs at bergstroem.nu or jbergstroem@irc.

[Fishrock123]

Sounds like a _rawDebug() bug. If anything should work, it's _rawDebug().

[jbergstroem]

Just to be clear then; what actually needs fixing is PrintErrorString.

[Trott]

Guess it's time for me to brush up a bit on my so-rusty-as-to-be-nonexistent C++ skillz....

[Trott]

If I change line 3066 of node.cc from...

PrintErrorString("%s\n", *message);

...to...

PrintErrorString("%s (%d bytes)\n", *message, message.length());

...and trigger the bug, the output looks like this:

Debugger listening on port 12390Debugger listening on port 12389 (32 bytes)
 (32 bytes)

The one thing this proves is that PrintErrorString() is getting called twice with the correct character strings. Otherwise, there wouldn't be two instances of (32 bytes) in the string. There would be one instance and the byte count would be around 64 bytes or so.

But how the heck are the lines getting interwoven like that?!

PrintErrorString() itself is very simple. With all the Windows stuff ifdef'ed out, it amounts to this:

static void PrintErrorString(const char* format, ...) {
  va_list ap;
  va_start(ap, format);
  vfprintf(stderr, format, ap);
  va_end(ap);
}

For fun, I tried adding fflush(stderr) after the vfprintf() call but that didn't fix anything.

My somewhat ignorant guesses are:

• Something weird going on where a carriage return or other weird character is somehow ending up in the line causing the display to somehow munge the two lines together. This seems unlikely to me, but what do I know?
• Some OS-specific bug or "feature" that causes stderr and/or vfprintf() to do this weirdness.

Does someone more knowledgable than me see something obvious I'm missing? I'm happy to keep pushing on this but it feels like something someone with more experience on the C++ side might see right away.

[Trott]

Replacing PrintErrorString() with fprintf() doesn't fix the issue. https://ci.nodejs.org/job/node-test-commit-smartos/221/nodes=smartos14-32/console

SmartOS bug/feature?

[Trott]

It looks like ultimately the problem is that stderr is unbuffered on SmartOS so we're getting some interleaving. Forcing it to line-buffering seems to fix it. Going to test some more and then submit a fix soon hopefully if the fix doesn't break a ton of other stuff in CI or something...

[Trott]

Closing in favor of #3701

[MylesBorins]

removing lts-watch tag as this did not land