From 2106ef000c7d891385d5480a61d95327caf8e277 Mon Sep 17 00:00:00 2001 From: isaacs Date: Sat, 2 Mar 2013 10:20:33 -0800 Subject: [PATCH] net: Provide better error when writing after FIN The stock writable stream "write after end" message is overly vague, if you have clearly not called end() yourself yet. When we receive a FIN from the other side, and call destroySoon() as a result, then generate an EPIPE error (which is what would happen if you did actually write to the socket), with a message explaining what actually happened. --- lib/_stream_writable.js | 11 +++- lib/net.js | 28 ++++++++- test/simple/test-socket-write-after-fin-error.js | 80 ++++++++++++++++++++++++ test/simple/test-socket-write-after-fin.js | 64 +++++++++++++++++++ 4 files changed, 179 insertions(+), 4 deletions(-) create mode 100644 test/simple/test-socket-write-after-fin-error.js create mode 100644 test/simple/test-socket-write-after-fin.js diff --git a/lib/_stream_writable.js b/lib/_stream_writable.js index b8f88be..62e0705 100644 --- a/lib/_stream_writable.js +++ b/lib/_stream_writable.js @@ -126,10 +126,15 @@ Writable.prototype.write = function(chunk, encoding, cb) { } if (state.ended) { + var self = this; var er = new Error('write after end'); - if (typeof cb === 'function') - cb(er); - this.emit('error', er); + // TODO: defer error events consistently everywhere, not just the cb + self.emit('error', er); + if (typeof cb === 'function') { + process.nextTick(function() { + cb(er); + }); + } return; } diff --git a/lib/net.js b/lib/net.js index 934f9f2..023d30b 100644 --- a/lib/net.js +++ b/lib/net.js @@ -240,8 +240,31 @@ function onSocketEnd() { this.read(0); } - if (!this.allowHalfOpen) + if (!this.allowHalfOpen) { + this.write = writeAfterFIN; this.destroySoon(); + } +} + +// Provide a better error message when we call end() as a result +// of the other side sending a FIN. The standard 'write after end' +// is overly vague, and makes it seem like the user's code is to blame. +function writeAfterFIN(chunk, encoding, cb) { + if (typeof encoding === 'function') { + cb = encoding; + encoding = null; + } + + var er = new Error('This socket has been closed by the other party'); + er.code = 'EPIPE'; + var self = this; + // TODO: defer error events consistently everywhere, not just the cb + self.emit('error', er); + if (typeof cb === 'function') { + process.nextTick(function() { + cb(er); + }); + } } exports.Socket = Socket; @@ -708,6 +731,9 @@ function connect(self, address, port, addressType, localAddress) { Socket.prototype.connect = function(options, cb) { + if (this.write !== Socket.prototype.write) + this.write = Socket.prototype.write; + if (typeof options !== 'object') { // Old API: // connect(port, [host], [cb]) diff --git a/test/simple/test-socket-write-after-fin-error.js b/test/simple/test-socket-write-after-fin-error.js new file mode 100644 index 0000000..1235574 --- /dev/null +++ b/test/simple/test-socket-write-after-fin-error.js @@ -0,0 +1,80 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +var common = require('../common'); +var assert = require('assert'); + +// This is similar to simple/test-socket-write-after-fin, except that +// we don't set allowHalfOpen. Then we write after the client has sent +// a FIN, and this is an error. However, the standard "write after end" +// message is too vague, and doesn't actually tell you what happens. + +var net = require('net'); +var serverData = ''; +var gotServerEnd = false; +var clientData = ''; +var gotClientEnd = false; +var gotServerError = false; + +var server = net.createServer(function(sock) { + sock.setEncoding('utf8'); + sock.on('error', function(er) { + console.error(er.code + ': ' + er.message); + gotServerError = er; + }); + + sock.on('data', function(c) { + serverData += c; + }); + sock.on('end', function() { + gotServerEnd = true + sock.write(serverData); + sock.end(); + }); + server.close(); +}); +server.listen(common.PORT); + +var sock = net.connect(common.PORT); +sock.setEncoding('utf8'); +sock.on('data', function(c) { + clientData += c; +}); + +sock.on('end', function() { + gotClientEnd = true; +}); + +process.on('exit', function() { + assert.equal(clientData, ''); + assert.equal(serverData, 'hello1hello2hello3\nTHUNDERMUSCLE!'); + assert(gotClientEnd); + assert(gotServerEnd); + assert(gotServerError); + assert.equal(gotServerError.code, 'EPIPE'); + assert.notEqual(gotServerError.message, 'write after end'); + console.log('ok'); +}); + +sock.write('hello1'); +sock.write('hello2'); +sock.write('hello3\n'); +sock.end('THUNDERMUSCLE!'); diff --git a/test/simple/test-socket-write-after-fin.js b/test/simple/test-socket-write-after-fin.js new file mode 100644 index 0000000..88d780b --- /dev/null +++ b/test/simple/test-socket-write-after-fin.js @@ -0,0 +1,64 @@ +// Copyright Joyent, Inc. and other Node contributors. +// +// Permission is hereby granted, free of charge, to any person obtaining a +// copy of this software and associated documentation files (the +// "Software"), to deal in the Software without restriction, including +// without limitation the rights to use, copy, modify, merge, publish, +// distribute, sublicense, and/or sell copies of the Software, and to permit +// persons to whom the Software is furnished to do so, subject to the +// following conditions: +// +// The above copyright notice and this permission notice shall be included +// in all copies or substantial portions of the Software. +// +// THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +// OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN +// NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, +// DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR +// OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE +// USE OR OTHER DEALINGS IN THE SOFTWARE. + +var common = require('../common'); +var assert = require('assert'); +var net = require('net'); +var serverData = ''; +var gotServerEnd = false; +var clientData = ''; +var gotClientEnd = false; + +var server = net.createServer({ allowHalfOpen: true }, function(sock) { + sock.setEncoding('utf8'); + sock.on('data', function(c) { + serverData += c; + }); + sock.on('end', function() { + gotServerEnd = true + sock.end(serverData); + server.close(); + }); +}); +server.listen(common.PORT); + +var sock = net.connect(common.PORT); +sock.setEncoding('utf8'); +sock.on('data', function(c) { + clientData += c; +}); + +sock.on('end', function() { + gotClientEnd = true; +}); + +process.on('exit', function() { + assert.equal(serverData, clientData); + assert.equal(serverData, 'hello1hello2hello3\nTHUNDERMUSCLE!'); + assert(gotClientEnd); + assert(gotServerEnd); + console.log('ok'); +}); + +sock.write('hello1'); +sock.write('hello2'); +sock.write('hello3\n'); +sock.end('THUNDERMUSCLE!'); -- 2.7.4