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

passing socket from master to worker causes a timeout issue where socket is null. #13435

Closed
japrescott opened this issue Jun 3, 2017 · 5 comments
Labels
cluster Issues and PRs related to the cluster subsystem. confirmed-bug Issues with confirmed bugs. http Issues or PRs related to the http subsystem. regression Issues related to regressions.

Comments

@japrescott
Copy link

japrescott commented Jun 3, 2017

node: 8
npm: 5.0.2
linux Ubuntu 1604 -> 4.8.0-51

Setup:
When using node with the cluster module while using socket.io, connections are passed from the master to the children by emitting. This is needed for "sticky-sessions" to work. This worked with node v7.10.

Since node8, I am getting following error;

worker process 3583 got 'uncaughtException', shutdown gracefully! TypeError: Cannot read property 'emit' of null
    at Socket.socketOnTimeout (_http_server.js:386:34)
    at emitNone (events.js:105:13)
    at Socket.emit (events.js:207:7)
    at Socket._onTimeout (net.js:401:8)
    at ontimeout (timers.js:488:11)
    at tryOnTimeout (timers.js:323:5)
    at Timer.listOnTimeout (timers.js:283:5) 'TypeError: Cannot read property \'emit\' of null\n    at Socket.socketOnTimeout (_http_server.js:386:34)\n    at emitNone (events.js:105:13)\n    at Socket.emit (events.js:207:7)\n
    at Socket._onTimeout (net.js:401:8)\n    at ontimeout (timers.js:488:11)\n    at tryOnTimeout (timers.js:323:5)\n    at Timer.listOnTimeout (timers.js:283:5)'

This is particularly strange as I have a domain wrapping the worker code which should prevent an uncaughtException.

I have my suspicion, this is related to #13348

@mscdex mscdex added cluster Issues and PRs related to the cluster subsystem. http Issues or PRs related to the http subsystem. labels Jun 3, 2017
@gavinaiken
Copy link

I have this problem too. I have a simple repro for the bug - use the script from https://github.com/elad/node-cluster-socket.io, i.e. this:

var express = require('express'),
    cluster = require('cluster'),
    net = require('net'),
    sio = require('socket.io'),
    sio_redis = require('socket.io-redis');

var port = 3000,
    num_processes = require('os').cpus().length;

if (cluster.isMaster) {
	// This stores our workers. We need to keep them to be able to reference
	// them based on source IP address. It's also useful for auto-restart,
	// for example.
	var workers = [];

	// Helper function for spawning worker at index 'i'.
	var spawn = function(i) {
		workers[i] = cluster.fork();

		// Optional: Restart worker on exit
		workers[i].on('exit', function(code, signal) {
			console.log('respawning worker', i);
			spawn(i);
		});
    };

    // Spawn workers.
	for (var i = 0; i < num_processes; i++) {
		spawn(i);
	}

	// Helper function for getting a worker index based on IP address.
	// This is a hot path so it should be really fast. The way it works
	// is by converting the IP address to a number by removing non numeric
  // characters, then compressing it to the number of slots we have.
	//
	// Compared against "real" hashing (from the sticky-session code) and
	// "real" IP number conversion, this function is on par in terms of
	// worker index distribution only much faster.
	var worker_index = function(ip, len) {
		var s = '';
		for (var i = 0, _len = ip.length; i < _len; i++) {
			if (!isNaN(ip[i])) {
				s += ip[i];
			}
		}

		return Number(s) % len;
	};

	// Create the outside facing server listening on our port.
	var server = net.createServer({ pauseOnConnect: true }, function(connection) {
		// We received a connection and need to pass it to the appropriate
		// worker. Get the worker for this connection's source IP and pass
		// it the connection.
		var worker = workers[worker_index(connection.remoteAddress, num_processes)];
		worker.send('sticky-session:connection', connection);
	}).listen(port);
} else {
    // Note we don't use a port here because the master listens on it for us.
	var app = new express();

	// Here you might use middleware, attach routes, etc.

	// Don't expose our internal server to the outside.
	var server = app.listen(0, 'localhost'),
	    io = sio(server);

	// Tell Socket.IO to use the redis adapter. By default, the redis
	// server is assumed to be on localhost:6379. You don't have to
	// specify them explicitly unless you want to change them.
	io.adapter(sio_redis({ host: 'localhost', port: 6379 }));

	// Here you might use Socket.IO middleware for authorization etc.

	// Listen to messages sent from the master. Ignore everything else.
	process.on('message', function(message, connection) {
		if (message !== 'sticky-session:connection') {
			return;
		}

		// Emulate a connection event on the server by emitting the
		// event with the connection the master sent us.
		server.emit('connection', connection);

		connection.resume();
	});
}

Run that script with node, and hit the server with Chrome (should see an error Cannot GET / returned). With node 7.10.0 the server keeps running fine. With node 8.0.0 I get this error 5s after making the request in the browser:

_http_server.js:386
  var serverTimeout = this.server.emit('timeout', this);
                                 ^

TypeError: Cannot read property 'emit' of null
    at Socket.socketOnTimeout (_http_server.js:386:34)
    at emitNone (events.js:105:13)
    at Socket.emit (events.js:207:7)
    at Socket._onTimeout (net.js:401:8)
    at ontimeout (timers.js:488:11)
    at tryOnTimeout (timers.js:323:5)
    at Timer.listOnTimeout (timers.js:283:5)
respawning worker 4

@Tjommel
Copy link

Tjommel commented Jun 9, 2017

Same here! worked with 7.x now wiht 8.x i get the same error message as @gavinaiken get.

@lpinca
Copy link
Member

lpinca commented Jun 9, 2017

This is probably my fault. In #11926 I wrongly assumed that socket.server is always set but in this case the 'connection' event is emitted manually with a socket argument whose server property is null.

Can you build node with this patch and see if it fixes the issue?

diff --git a/lib/_http_server.js b/lib/_http_server.js
index 357400e350..aae55a5282 100644
--- a/lib/_http_server.js
+++ b/lib/_http_server.js
@@ -292,6 +292,9 @@ Server.prototype.setTimeout = function setTimeout(msecs, callback) {
 function connectionListener(socket) {
   debug('SERVER new http connection');
 
+  if (socket.server === null)
+    socket.server = this;
+
   httpSocketSetup(socket);
 
   // If the user has added a listener to the server,

@lpinca lpinca added the confirmed-bug Issues with confirmed bugs. label Jun 9, 2017
@gavinaiken
Copy link

Just tested, was able to recreate the bug with node built from master, and that patch does seem to fix it 👍

@lpinca
Copy link
Member

lpinca commented Jun 9, 2017

I'm working on a PR, almost done.

lpinca added a commit to lpinca/node that referenced this issue Jun 9, 2017
Fixes a regression that caused an error to be thrown when trying to
emit the 'timeout' event on the server referenced by `socket.server`.

Fixes: nodejs#13435
Refs: nodejs#11926
@lpinca lpinca added the regression Issues related to regressions. label Jun 10, 2017
addaleax pushed a commit that referenced this issue Jun 12, 2017
Fixes a regression that caused an error to be thrown when trying to
emit the 'timeout' event on the server referenced by `socket.server`.

Fixes: #13435
Refs: #11926
PR-URL: #13578
Reviewed-By: Colin Ihrig <[email protected]>
JaoodxD added a commit to JaoodxD/NodeServer that referenced this issue Nov 15, 2023
To prevent master process reading from socket which cause hanging we have to pause connection and manualy resume when child_process will be ready to process it.

Mentions in Node.js documentation here [net.createSerer](https://nodejs.org/api/net.html#netcreateserveroptions-connectionlistener)
```
If pauseOnConnect is set to true, then the socket associated with each incoming connection will be paused, and no data will be read from its handle. This allows connections to be passed between processes without any data being read by the original process. To begin reading data from a paused socket, call socket.resume().  
```

The issue resolved with a help of following issue: nodejs/node#13435
tshemsedinov pushed a commit to HowProgrammingWorks/NodeServer that referenced this issue Nov 15, 2023
To prevent master process reading from socket which cause hanging we have to pause connection and manualy resume when child_process will be ready to process it.

Mentions in Node.js documentation here [net.createSerer](https://nodejs.org/api/net.html#netcreateserveroptions-connectionlistener)
```
If pauseOnConnect is set to true, then the socket associated with each incoming connection will be paused, and no data will be read from its handle. This allows connections to be passed between processes without any data being read by the original process. To begin reading data from a paused socket, call socket.resume().  
```

The issue resolved with a help of following issue: nodejs/node#13435
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cluster Issues and PRs related to the cluster subsystem. confirmed-bug Issues with confirmed bugs. http Issues or PRs related to the http subsystem. regression Issues related to regressions.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants