Access to @ShogunPanda to test-nearform_intel-ubuntu1604-x64

[ShogunPanda]

Please grant me access to the machine above, I'm trying to debug why jobs like

https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/1154/

always get stuck on the http simple benchmark.

[Trott]

@nodejs/build

[Trott]

+1 as long as that's not the only machine available for any particular Jenkins tasks

[mhdawson]

I checked and there are 2 machines that the micro-benchmarks can run so +1 from me as well.

[sxa]

No real objection from me, but is this problem specific to those machines or could it be reproduced on any xLInux system?

[Trott]

Ref: #2656

@rvagg pointed out that we're running 16.04 on the benchmark machine and maybe we should consider upgrading and seeing if it fixes the problem or not.

[rvagg]

@ShogunPanda I've added your keys (two, from your GitHub) to the machine: ssh root@92.51.196.115, let us know when you're done.

IF it looks like this might be related to us running an older version of Ubuntu then you could bail and let us know and we can prioritise getting it upgraded. It's a TODO that really needs to get done, and as @Trott said, it may solve the issue for us. But it's upgrading can be a traumatic exercise so pulling the trigger on that hasn't been easy. But maybe you're giving us reason to just go ahead and get it done.

[ShogunPanda]

Update, I was able to easily reproduce the issue and, after playing with node a little bit (and thanks to wtf-node) I noticed that the child process never exits because one of the connections is still active. wrk process has terminated but somehow the connection is still showing up as active.

A specific fix for benchmark/http/simple.js was to add server.closeAllConnections before server.close, but this problem might reappear in the future somewhere else.
Instead, I was thinking to change the child process branch in https://github.com/nodejs/node/blob/main/benchmark/common.js#L289 as follows:

function sendResult(data) {
  if (process.send) {
    // If forked, report by process send
    process.send(data, () => process.exit(0)); // This the line I plan to to change.
  } else {
    // Otherwise report by stdout
    process.stdout.write(formatResult(data));
  }
}

As after sending the results we don't care whatever else the process is doing.
Do you agree?

[rvagg]

it's a fix at least .. not very elegant but I don't see an obvious other place to put it - unless you added a callback from sendResult or made it async and then used that at the call site, maybe at the bottom of end() where you await sendResult(...); if (process.send) process.exit(0); - but maybe that's not a whole lot different.

[ShogunPanda]

@rvagg I went for slightly better approach: rather than always close the process, I give it a little time to gracefully exit, otherwise I proceed.

[rvagg]

access removed

thanks for diagnosing it @ShogunPanda