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

'end' event not called on connected socket wrapped with tls #10871

Open
zivbr opened this issue Jan 18, 2017 · 11 comments
Open

'end' event not called on connected socket wrapped with tls #10871

zivbr opened this issue Jan 18, 2017 · 11 comments
Labels
stream Issues and PRs related to the stream subsystem. tls Issues and PRs related to the tls subsystem.

Comments

@zivbr
Copy link

zivbr commented Jan 18, 2017

  • Version: v6.9.4, v7.4.0
  • Platform: Linux pc 4.2.0-30-generic docs: re-word project messaging #36-Ubuntu SMP Fri Feb 26 00:58:07 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux
  • Subsystem: net, tls

When wrapping an existing socket with tls, the socket's 'end' event is not called if the socket was already connected.

Example:

In this case 'end' will be called:

var Socket = require('net').Socket;
var tls = require('tls');

var HOST = 'www.google.com';

var socket = new Socket();
var tlsOptions = {
  host: HOST,
  socket: socket
};

socket.on('close', function() { console.log('close'); });
socket.on('end', function() { console.log('end'); });

var tlsSocket = tls.connect(tlsOptions, function() {
  console.log('connected');
  tlsSocket.end();
});

socket.connect({
  port: 443,
  host: HOST
});

In this case 'end' will not be called:

var Socket = require('net').Socket;
var tls = require('tls');

var HOST = 'www.google.com';

var socket = new Socket();
var tlsOptions = {
  host: HOST,
  socket: socket
};

socket.on('close', function() { console.log('close'); });
socket.on('end', function() { console.log('end'); });

socket.connect({
  port: 443,
  host: HOST
});

var tlsSocket = tls.connect(tlsOptions, function() {
  console.log('connected');
  tlsSocket.end();
});
@mscdex mscdex added the tls Issues and PRs related to the tls subsystem. label Jan 18, 2017
@webertrlz
Copy link

webertrlz commented Jan 18, 2017

*I'm not sure if this is related because the end event is emitted when the other end sends a fin
package, but I'd like to understand why this happens if there is a discussion on this topic.

in this piece of code, the 'end' event is also not emitted if we hit CTRL^C on the Client side:

const server = require('net').createServer((socket)=>{
	var interval =	setInterval(()=>{ socket.write("hello socket"); }, 10);
	socket.on('end', ()=>{ console.log('end'); });
	socket.on('close', ()=>{ console.log('close'); clearInterval(interval); });
	socket.on('error', (error)=>{ console.log(error);	});
}).listen(9999);

then on a shell do: telnet 0 9999
wait a few and then hit CTRL^C then close telnet (with CTRL^]q)

if we don't hit CTRL^C and just close the telnet client, the end event is emitted.

@Trott
Copy link
Member

Trott commented Jul 16, 2017

@nodejs/crypto @nodejs/http

@ajimix
Copy link

ajimix commented Feb 21, 2018

I'm having the same problem with node 8.9.4

@oyyd
Copy link
Contributor

oyyd commented Oct 3, 2018

According to the code here:

node/lib/_tls_wrap.js

Lines 312 to 315 in 097896b

if ((socket instanceof net.Socket && socket._handle) || !socket)
wrap = socket;
else
wrap = new StreamWrap(socket);

A net.Socket instance that doesn't call connect will be wrapped in a StreamWrap as the socket._handle here will be null.

The problem is that a socket not wrapped in StreamWrap won't emit data, drawn, end events because its TCP handle will be "occupied" by a TLSWrap. However, events like lookup, connect, ready, close still works. Whether the net.Socket is wrapped in StreamWrap or not will make it behave differently.

@oyyd
Copy link
Contributor

oyyd commented Oct 15, 2018

Considering how TLSWrap and StreamBase work, I guess it's not ideal to "fix" this issue as it may result in reduced performance.
Edit: Find that http2 also use StreamWrap so that I guess it's okay.

Personally, for anyone has the same problem, directly use tlsSocket if possible. Otherwise, wrap your net.Socket with stream.Duplex.

@jasnell jasnell added the stream Issues and PRs related to the stream subsystem. label Jun 26, 2020
@jasnell
Copy link
Member

jasnell commented Jun 26, 2020

@ronag ... this may be one that interests you. It could be this is already fixed but not likely, but it falls in line with making all the streams things work consistently.

@ronag
Copy link
Member

ronag commented Jun 26, 2020

I will take a look.

@ronag
Copy link
Member

ronag commented Jun 26, 2020

It's still a problem. Or at least it still seems to behave in the same way as when OP created issue.

@ronag
Copy link
Member

ronag commented Jun 26, 2020

I'm actually more surprised that end is emitted in one of the cases.

@ronag
Copy link
Member

ronag commented Jun 26, 2020

@jasnell: I find that 'end' makes very little sense for sockets at the moment, since we don't really have a graceful vs non-graceful connection disconnect. So whether an when 'end' is emitted is (for me) somewhat pseudo random at the moment in general, i.e. we don't have well defined semantics for this. Or at least not that I'm familiar with.

Would be interesting to understand why there is a difference between OP's examples and why they differ. However, not sure what to do what that information.

At the moment I would recommend not using 'end' at all when working with sockets and only use 'close'.

Refs: #31916

@ronag ronag mentioned this issue Jun 28, 2020
25 tasks
@ronag
Copy link
Member

ronag commented Aug 2, 2020

At the moment I would recommend not using 'end' at all when working with sockets and only use 'close'.

@jasnell @addaleax Thoughts on the above? Should we document it?

mnvr added a commit to ente-io/ente that referenced this issue Jun 26, 2024
mnvr added a commit to ente-io/ente that referenced this issue Jun 26, 2024
This caused the reference counts to not be zero when we'd go to clear
the cache in `clearPendingUploads`.

Bug introduced in the nightly build, but didn't have any negative impact
except printing an error in the logs because of the unhandled promise
rejection.

Ref: nodejs/node#10871 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Development

No branches or pull requests

8 participants