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

Namespace inconsistency : root namespace is gobbling anything #2124

Closed
joe-mojo opened this issue May 18, 2015 · 11 comments
Closed

Namespace inconsistency : root namespace is gobbling anything #2124

joe-mojo opened this issue May 18, 2015 · 11 comments

Comments

@joe-mojo
Copy link

If I create the following namespaces :
/test
/status
/status/db
and call namespace.on('connection', handler) with a different handler for each namespace, client connecting to /test are handled by /test handler, /status by /status handler, and the most important, /status/db client are handled by /status/db and NOT /status handler.
This means sub-namespaces are independents.

So why root namespaces is gobbling any other sub-namespsce ?? This is inconsistent !
If I created the following namespaces :
/
/status
and call namespace.on('connection', handler) with a different handler for each namespace, only the handler for "/" is called, even if client is specifying "/status".
This is an issue, and create another oddity: If create only "/" namespace, as expected client connecting to "/status" get an error. BUT if I create both "/" and "/status" namespaces with 2 different handlers, client connecting to "/status" are accepted but their events are routed to "/" handler !

@jordanpappas
Copy link

I understand your frustration with nesting the namespaces, but this is not the functionality of the namespace. The point is to "separate concerns" while minimizing tcp connections. However, lets forget this for a minute.

With just two namespaces, a "root" default one / and a "sub" on (/sub). It is true that the default namespace handler is called as well as the sub handler, but the events are not routed through the root handler. Try this example and you'll see:

// server
var io = require('socket.io')(3000);

io.on('connection', function(socket) {
  console.log('root called');
  socket.on('test', function() {console.log("ROOT")})
});

var sub = io.of('/sub');
sub.on('connection', function(socket) {
  console.log('sub called');
  socket.on('test', function() {console.log("SUB")});
});

// client
var io = require('socket.io-client');
var socket2 = io.connect('http://localhost:3000/sub');

setTimeout(function() {
    socket2.emit('test');
},3000);

In three seconds you will see SUB on the server side, not ROOT. So why do you think they are being routed through the "/" handler?

Back to your other frustrations: If you need to separate a namespace out further, you should use rooms. The best thing to do to keep this from being confusing is to ditch the root namespace and put each separate concern in its own namespace, and use rooms to divvy this up even more. Even better, just use the root namespace for initialization/cleanup.

@joe-mojo
Copy link
Author

Sorry but by debugging and "console.logging" in my app, I see that If you put a single io.of("/"), any connection is using middleware and handler associated with / and NOT their respective handler:

var server = require('http').createServer(function handler(req, res) {
    logger.debug('handler: req=%s', req.url, {});
    res.end();
});

var io = require('socket.io')(server);
// Enable only the websocket transport
io.set('transports', ['websocket']);
var handlers = require('eventhandler');
var statusHandler = require('statushandler');
var generalNsp = io.of('/');
generalNsp.use(handlers.handshake.bind(handlers));
generalNsp.on('connection', handlers.onConnection.bind(handlers));
var statusNsp = io.of('/status');
statusNsp.on('connection', statusHandler.onConnection.bind(statusHandler));
server.listen(config.wsserver.port, function onListening() {
    var port = server.address().port;
    var origin = 'http://localhost:' + port;
    console.log('Server started on: ', origin);
});

If generalNsp is '/', handlers.handshake middleware is called even if client required /status.
if generalNsp is '/whatever', handlers.handshake middleware is called ONLY on /whatever and not /status (as expected).

I don't need rooms, I just need that client connecting to "/" use hanlders.* callbacks, and client connecting to "/status" use statusHandler.* callbacks.

@joe-mojo
Copy link
Author

@jordanpappas I just isolated the bug with you own code. See :

1 first test : create "/root" and "/sub" namespaces, client connecting to "/sub" :

// server
var io = require('socket.io')(3000);
var rootNsp = io.of('/root')
rootNsp.use(function(client, next){
    console.log("root middleware called");
    next();
});
rootNsp.on('connection', function(socket) {
    console.log('root called');
    socket.on('test', function() {console.log("ROOT")})
});

var sub = io.of('/sub');
sub.on('connection', function(socket) {
    console.log('sub called');
    socket.on('test', function() {console.log("SUB")});
});

// client
var io = require('socket.io-client');
var socket2 = io.connect('http://localhost:3000/sub');

setTimeout(function() {
    socket2.emit('test');
},3000);

The above code will produce (as expected) :

sub called
SUB

2 create "/root" and "/root/sub" namespaces :

// server
var io = require('socket.io')(3000);
var rootNsp = io.of('/root')
rootNsp.use(function(client, next){
    console.log("root middleware called");
    next();
});
rootNsp.on('connection', function(socket) {
    console.log('root called');
    socket.on('test', function() {console.log("ROOT")})
});

var sub = io.of('/root/sub');
sub.on('connection', function(socket) {
    console.log('sub called');
    socket.on('test', function() {console.log("SUB")});
});

// client
var io = require('socket.io-client');
var socket2 = io.connect('http://localhost:3000/root/sub');

setTimeout(function() {
    socket2.emit('test');
},3000);

The above code outputs the same, as expected :

sub called
SUB

3 the root bug : create "/" and "/sub" namespaces, and connect to "/sub":

// server
var io = require('socket.io')(3000);
var rootNsp = io.of('/')
rootNsp.use(function(client, next){
    console.log("root middleware called");
    next();
});
rootNsp.on('connection', function(socket) {
    console.log('root called');
    socket.on('test', function() {console.log("ROOT")})
});

var sub = io.of('/sub');
sub.on('connection', function(socket) {
    console.log('sub called');
    socket.on('test', function() {console.log("SUB")});
});

// client
var io = require('socket.io-client');
var socket2 = io.connect('http://localhost:3000/sub');

setTimeout(function() {
    socket2.emit('test');
},3000);

The above code outputs :

root middleware called
root called
sub called
SUB

... and that is not expected. As you see in the /root/sub example, parent path are never called when connecting to a child. So it is unexpected that connection to "/sub" triggers "/" middlewares&handlers.

So the default route is inconsistent, and this inconsistent behaviour is not documented on the terse socket.io doc, that is not covering extensively all what you can do with namespaces. You need either to fix this, either document it explicitly and give an option to avoid it.

@ghost
Copy link

ghost commented Jun 1, 2015

The same problem also appears on 'disconnect' event.

@darrachequesne
Copy link
Member

@joe-mojo Hi! As far as I understand, the server initially connects the incoming client to the '/' namespace, and then connects it to '/sub' (as the client emit a new packet to ask the connection to a particular namespace):
From https://github.com/socketio/socket.io/blob/master/lib/index.js

Server.prototype.bind = function(engine){
  this.engine = engine;
  this.engine.on('connection', this.onconnection.bind(this)); // connection event
  return this;
};
Server.prototype.onconnection = function(conn){
  debug('incoming connection with id %s', conn.id);
  var client = new Client(this, conn);
  // initial connection to '/', middleware & handlers are called
  client.connect('/');
  return this;
};

And then, from https://github.com/socketio/socket.io/blob/master/lib/client.js

Client.prototype.ondecoded = function(packet) {
  if (parser.CONNECT == packet.type) {
    // connection to '/sub', middleware & handlers are called
    this.connect(packet.nsp);
  }
  [...]

So isn't it the expected behaviour? Maybe the documentation just needs to be improved here, as it simply states By default the client always connects to /.

@pist One could actually add:

Client.prototype.connect = function(name){
  debug('connecting to namespace %s', name);
  if (!this.server.nsps[name]) {
    this.packet({ type: parser.ERROR, nsp: name, data : 'Invalid namespace'});
    return;
  }
  var nsp = this.server.of(name);
  if ('/' != name && !this.nsps['/']) {
    this.connectBuffer.push(name);
    return;
  }
  // [ADDED] disconnect from the initial namespace
  if (Object.keys(this.nsps).length == 1 && this.nsps['/']) {
    this.nsps['/'].disconnect();
  }

@joe-mojo
Copy link
Author

joe-mojo commented Oct 4, 2015

@darrachequesne Sorry but my example shown an inconstitency. The code is not the reference, the contract is.
There is 2 ways of fixing this :

  • either warn clearly in the doc that "/" is a special namespace, that is ALWAYS called whatever the client connected to. This is important, because if you go to prod without knowing this, you won't be able to differenciate namespace on connection when you will add namespaces later. I'm in this case. This is very inconvenient, because I had to use query string to make the difference.
  • or fix the code to avoir the connection handlers on "/" namespace to be called when connecting to "/sub", and so it will be consistent with the "/ns" and "/ns/sub" case.

While the code says something that is not expected (but very important because it can be a point of no return), the API doc miss that point (by the way the doc is incomplete for other points too).

My point of view, as a socket.io user, is that I found a bug in code because of the inconsistency. But It would have been OK if I saw a big warn about the "/" namespace because it would have avoided the pitfall.

@ghost
Copy link

ghost commented Oct 4, 2015

Actually there is such an information about namespace in the documentation:
"By default the client always connects to /." but I agree that it is the lack of consistency :(

@joe-mojo
Copy link
Author

joe-mojo commented Oct 4, 2015

"By default the client always connects to /." doesn't mean "if you declare a / namespace along with another, the connection handler of / will always be called". I understand "by default" here as "if you don't specify namespace".

@perrin4869
Copy link
Contributor

+1 agree with @joe-mojo I understood the line there in the exact same way, tripped over it for quite a while.
I think that the second option, avoiding connecting to the / altogether would be easier to implement and be more consistent than the current solution

@fourpixels
Copy link

@jordanpappas can you please explain what you've meant here: The point is to "separate concerns" while minimizing tcp connections.?

Each call to a new namespace creates new socket, right? Or at least that's what I'm seeing right now :)

Otherwise I agree that docs needs improvement, as I also thought that connects to '/' namespace by default meant it just got there, not that it acts like the base namespace of all :)

@darrachequesne
Copy link
Member

That issue was closed automatically. Please check if your issue is fixed with the latest release, and reopen if needed (with a fiddle reproducing the issue if possible).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants
@nkzawa @joe-mojo @perrin4869 @fourpixels @jordanpappas @darrachequesne and others