net: always invoke after-write callback #24291
Conversation
This is part of the streams API contract, and aligns network sockets with other streams in this respect.
|
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/ |
I was thinking this when I approved the PR, but didn't mention it. I think I'd be +1 to semver-major. |
|
I would prefer to have a test on net that verifies this. |
|
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/ |
|
@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();
})); |
|
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 <
notifications@github.com> 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>
.
|
|
Sigh, CITGM didn’t work again. New attempt: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1617/ (scheduled) |
|
CITGM rebuild with CITGM_COMMAND = https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1626/ |
|
Landed in c347e77 I’ve added the dont-land-on-lts labels, if that’s okay with everyone. |
This is part of the streams API contract, and aligns network sockets with other streams in this respect. PR-URL: #24291 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This is part of the streams API contract, and aligns network sockets with other streams in this respect. PR-URL: #24291 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Weijia Wang <starkwang@126.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
This is part of the streams API contract, and aligns
network sockets with other streams in this respect.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes