Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

close event emitted on net socket but not tls #24984

Closed
davedoesdev opened this issue Dec 12, 2018 · 7 comments
Closed

close event emitted on net socket but not tls #24984

davedoesdev opened this issue Dec 12, 2018 · 7 comments
Labels
confirmed-bug Issues with confirmed bugs. tls Issues and PRs related to the tls subsystem.

Comments

@davedoesdev
Copy link
Contributor

Why does this emit a close event:

const fs = require('fs');
const path = require('path');
const net = require('net');
const server = net.createServer();

server.on('connection', function (conn) {
    conn.on('close', function () {
        console.log("CLOSE");
    });

    conn.resume();
    conn.end(Buffer.alloc(1024*1024));
});

server.listen(7000, function () {
    net.connect({
        port: 7000
    }, function () {
        this.end();
    });
});

but this does not emit a close event:

const fs = require('fs');
const path = require('path');
const tls = require('tls');
const server = tls.createServer({
    key: fs.readFileSync(path.join(__dirname, 'server.key')),
    cert: fs.readFileSync(path.join(__dirname, 'server.pem'))
});

server.on('secureConnection', function (conn) {
    conn.on('close', function () {
        console.log("CLOSE");
    });

    conn.resume();
    conn.end(Buffer.alloc(1024*1024));
});

server.listen(7000, function () {
    tls.connect({
        ca: fs.readFileSync(path.join(__dirname, 'ca.pem')),
        port: 7000
    }, function () {
        this.end();
    });
});

?

@lpinca
Copy link
Member

lpinca commented Dec 12, 2018

They both don't emit 'close' on my machine and the reason is that data is not read on the client. It works if this.resume() is added on the client.

@davedoesdev
Copy link
Contributor Author

OS TCP buffer size perhaps then

@davedoesdev
Copy link
Contributor Author

@lpinca what about this? I don't get a close event.

const fs = require('fs');
const path = require('path');
const tls = require('tls');
const server = tls.createServer({
    key: fs.readFileSync(path.join(__dirname, 'server.key')),
    cert: fs.readFileSync(path.join(__dirname, 'server.pem'))
});
let cconn = null;
let sconn = null;

function doit() {
    if (cconn && sconn) {
        cconn.resume();
        sconn.resume();
        sconn.end(Buffer.alloc(1024*1024));
        cconn.end();
    }
}

server.on('secureConnection', function (conn) {
    conn.on('close', function () {
        console.log("CLOSE");
    });
    sconn = conn;
    doit();
});

server.listen(7000, function () {
    tls.connect({
        ca: fs.readFileSync(path.join(__dirname, 'ca.pem')),
        port: 7000
    }, function () {
        cconn = this;
        doit();
    });
});

@davedoesdev
Copy link
Contributor Author

This fixes it:

diff --git a/lib/net.js b/lib/net.js
index 0229e450fc..377dd202af 100644
--- a/lib/net.js
+++ b/lib/net.js
@@ -371,8 +371,8 @@ Socket.prototype._final = function(cb) {
 };
 
 
-function afterShutdown(status, handle) {
-  var self = handle[owner_symbol];
+function afterShutdown(status) {
+  var self = this.handle[owner_symbol];
 
   debug('afterShutdown destroyed=%j', self.destroyed,
         self._readableState);

@lpinca
Copy link
Member

lpinca commented Dec 13, 2018

I can reproduce the issue, both 'end' and 'finish' are emitted but not 'close'.

@lpinca lpinca added confirmed-bug Issues with confirmed bugs. net Issues and PRs related to the net subsystem. labels Dec 13, 2018
@lpinca
Copy link
Member

lpinca commented Dec 13, 2018

cc: @addaleax

@lpinca lpinca added tls Issues and PRs related to the tls subsystem. and removed net Issues and PRs related to the net subsystem. labels Dec 13, 2018
@lpinca
Copy link
Member

lpinca commented Dec 13, 2018

The first bad commit seems to be e82f67d710.

@lpinca lpinca closed this as completed in 86e2ec4 Dec 26, 2018
targos pushed a commit that referenced this issue Jan 1, 2019
'close' event isn't emitted on a TLS connection if it's been written to
(but 'end' and 'finish' events are).

PR-URL: #25026
Fixes: #24984
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
refack pushed a commit to refack/node that referenced this issue Jan 14, 2019
'close' event isn't emitted on a TLS connection if it's been written to
(but 'end' and 'finish' events are).

PR-URL: nodejs#25026
Fixes: nodejs#24984
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
davedoesdev added a commit to davedoesdev/centro that referenced this issue Feb 8, 2019
BethGriggs pushed a commit that referenced this issue Apr 17, 2019
'close' event isn't emitted on a TLS connection if it's been written to
(but 'end' and 'finish' events are).

PR-URL: #25026
Fixes: #24984
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BethGriggs pushed a commit that referenced this issue Apr 28, 2019
'close' event isn't emitted on a TLS connection if it's been written to
(but 'end' and 'finish' events are).

PR-URL: #25026
Fixes: #24984
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BethGriggs pushed a commit that referenced this issue May 10, 2019
'close' event isn't emitted on a TLS connection if it's been written to
(but 'end' and 'finish' events are).

PR-URL: #25026
Fixes: #24984
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this issue May 16, 2019
'close' event isn't emitted on a TLS connection if it's been written to
(but 'end' and 'finish' events are).

PR-URL: #25026
Fixes: #24984
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants