OK, updated to just remove confusing unused object from function calls.

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

One buildbot failure but otherwise looks good. /cc @nodejs/testing (looking for an LGTM or two)

LGTM

LGTM

Shouldn't send check the type of callback right away?

@orangemocha You'd think so, but it doesn't. Not sure if that's a feature or a bug. Here's the current relevant code from lib/internal/child_process.js:

  target.send = function(message, handle, callback) {
    if (typeof handle === 'function') {
      callback = handle;
      handle = undefined;
    }
    if (this.connected) {
      return this._send(message, handle, false, callback);
    }
    const ex = new Error('channel closed');
    if (typeof callback === 'function') {
      process.nextTick(callback, ex);
    } else {
      this.emit('error', ex);  // FIXME(bnoordhuis) Defer to next tick.
    }
    return false;
  };

So, if the connected property is not set, it will emit an error event if callback is not a function. That's as expected, sort of. I mean, I expect that behavior if I don't send a callback at all (i.e., callback is null or undefined). But I would expect a TypeError if callback is a string or a (non-null) object.

But if it is connected, then it passes callback (whether it is a function or not) on to _send(). And, if the message is successfully sent, it does this:

        if (typeof callback === 'function')
          callback(null);

It will call the callback on success if the callback is a function, but ignore it otherwise.

Again, not sure if this is a bug or a feature (or me misunderstanding how the code works--always a possibility).

@orangemocha I should add that if _send() encounters an error, it basically does the same thing as send(): Calls the callback with the error if the callback is a function, otherwise emits an error event. The arguably surprising part is that send() and _send() treat various non-function values (e.g., strings, objects) the same as values that would indicate no callback parameter at all. Least astonishment principle would seem to indicate an error (or at least a warning?) occur if you send a string or an object instead of a function for the callback parameter. I would expect a TypeError.

Green! \o/ https://ci.nodejs.org/job/node-test-commit/1938/

Landed in 0351b2f

Added the lts-watch-v4.x label.

@Trott this commit is breaking the unit test suite on v4.x-staging

Would you be open to backporting this and opening a new PR so we can test and what not

@thealphanerd Odd. This change should be a no-op. When you say "unit test suite", what is that?

never mind.. there is a new flaky test on 4.x... I'll land this now

Path: parallel/test-http-destroyed-socket-write2
assert.js:89
  throw new assert.AssertionError({
  ^
AssertionError: false == true
    at test (/Users/thealphanerd/code/node-v4/test/parallel/test-http-destroyed-socket-write2.js:89:5)
    at Immediate.write [as _onImmediate] (/Users/thealphanerd/code/node-v4/test/parallel/test-http-destroyed-socket-write2.js:29:7)
    at processImmediate [as _immediateCallback] (timers.js:383:17)

Here's the error we are getting now

@thealphanerd I've not seen that before. (It's almost definitely unrelated to this PR.) Is that OS X? Are you running make test or doing something else?

happening locally on osx when running make-test.

Once I have this RC ready to cut I'm going to stress test that one

I'm pretty sure I've seen that one on OS X too

Oh, I sent a PR some time ago for this: #4970