net: always invoke after-write callback

[addaleax]

This is part of the streams API contract, and aligns
network sockets with other streams in this respect.

Checklist

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

[nodejs-github-bot]

@addaleax build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1571/pipeline

[addaleax]

I’d like to land this today or tomorrow.

@nodejs/tsc @nodejs/streams Just pointing out that this does change a test, and I won’t mind if anybody wants to label this semver-major because of it.

CI: https://ci.nodejs.org/job/node-test-pull-request/18554/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1613/

[cjihrig]

I won’t mind if anybody wants to label this semver-major because of it

I was thinking this when I approved the PR, but didn't mention it. I think I'd be +1 to semver-major.

[mcollina]

LGTM, let’s consider it a bugfix. I would be careful with backporting.

A CITGM run would be helpful.

[mcollina]

I would prefer to have a test on net that verifies this.

[addaleax]

The previous CITGM run had infra issues similar to what we’ve seen before, cc @nodejs/build-infra again

CITGM rebuild: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1614/

[addaleax]

@mcollina I’m not sure how to write a simple net-only test for this that doesn’t have the complexity of the test that is being modified here. This doesn’t seem to do the job:

constserver=net.createServer(common.mustCall((connection)=>{connection.end('foo');})).listen(0,common.mustCall(()=>{constclient=net.connect(server.address().port);client.write('bar',common.mustCall(()=>server.close()));// client.end();client.destroy();}));

[mcollina]

No problem, thanks for catching this anyway! I’ve been trying to hunt this for some time. Il giorno lun 12 nov 2018 alle 21:56 Anna Henningsen < [email protected]> ha scritto:

…

@mcollina <https://github.com/mcollina> I’m not sure how to write a simple net-only test for this that doesn’t have the complexity of the test that is being modified here. This doesn’t seem to do the job: const server = net.createServer(common.mustCall((connection) =>{connection.end('foo')})).listen(0, common.mustCall(() =>{const client = net.connect(server.address().port); client.write('bar', common.mustCall(() => server.close())); // client.end(); client.destroy()})); — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#24291 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AADL42uC3BSeR2jUqanrgJABPf4Bri9hks5uucmNgaJpZM4YYLzN> .

[addaleax]

Sigh, CITGM didn’t work again.

New attempt: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1617/ (scheduled)

[Trott]

CITGM rebuild with CITGM_COMMAND = citgm-all -J --excludeTags graceful-fs (h/t @refack):

https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1626/

[addaleax]

Landed in c347e77

I’ve added the dont-land-on-lts labels, if that’s okay with everyone.