http2: fix ping callback

[BridgeAR]

In case there was no ack, the callback would have returned
more than the error as return value. This makes sure that is not
the case anymore.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[BridgeAR]

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

[BridgeAR]

@nodejs/http @nodejs/http2 PTAL

[jasnell]

Passing the payload is intentional here as that is what is used to correlate. I'm -1 to removing it.

[BridgeAR]

@jasnell can you please elaborate? IMO this is against the default Node.js callback standard. In case of an error we normally only return the error and nothing else.

[jasnell]

function pingCallback(err, duration, payload) {
  if (err) {
    console.log(`Ping with payload "${payload.toString()}" failed`);
  } else {
    console.log(`Ping with payload "${payload.toString()}" succeeded with duration ${duration}`);
  }
}

session.ping(Buffer.from('12345678'), pingCallback);
session.ping(Buffer.from('87654321'), pingCallback);

[BridgeAR]

To me this is very surprising to rely on any arguments if an error was returned.

Any other opinions @nodejs/http2 @nodejs/tsc?

[addaleax]

As long as it’s documented this way, what are the issues with providing more information than just the error?

[BridgeAR]

Somewhat related: #20335

The documentation states:

The callback will be invoked with three arguments: an error argument that will be null if the PING was 
successfully acknowledged, a duration argument that reports the number of milliseconds elapsed since 
the ping was sent and the acknowledgment was received, and a Buffer containing the 8-byte PING 
payload.

The payload will be undefined in case of an error and there is no ack, so there should also not be a elapsed time, but the time is actually returned in case of an error. I do not even know what the returned time should now stand for and that seems like a bug to me.

Aside from that I expect all Node.js callbacks to always only return the error in an error case. Otherwise it seems like Schrödinger's cat to me. You get some results but also an error. What should you now work with?

[mcollina]

LGTM

[BridgeAR]

Landed in bb546ac and 29cddb4