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

broadcast doesn't work in namespace #291

Closed
outsideris opened this issue Jun 25, 2011 · 29 comments · Fixed by #340
Closed

broadcast doesn't work in namespace #291

outsideris opened this issue Jun 25, 2011 · 29 comments · Fixed by #340
Labels
bug Something isn't working

Comments

@outsideris
Copy link

var namespace = io
  .of('/namespace')
  .on('connection', function(socket) {
    socket.on('message', function(data) {
      socket.broadcast.send(data);
    }); 
  });

message event is fired...
but it can't broadcast. and broadcast.emit too..

I use socket.io .0.7.2 and node. 0.4.8

@midix
Copy link

midix commented Jun 25, 2011

Yes, I stumbled uppon it too.

I guess, such broadcasting is a bit incorrect. Logically it seems, the socket (which comes as an argument af connection event) should be able only to send message to the connected client and not to all of the clients of the namespace. For a simple 'send' it seems this is the way it works - if I do the socket.send(), only the connected client gets the message, and this is the behaviour I expected.
So logically it seems that the broadcast flag should be a feature of the namespace and not of the socket. But if I try the following code:

var chat = ioserver.of('/chat');

chat.on('connection', function (socket) {
      console.log('chat connection');   
      socket.on('message', function (msg) {
        console.log('chat message');  
        chat.broadcast.send(msg);
      });
  });

I get an exception:
'Cannot call method 'send' of undefined.' So the broadcast is not defined for a namespace and it will not work.

BTW, isn't there the same issue related error on the socket.io page in this example:

var io = require('socket.io').listen(80);

var chat = io
  .of('/chat');  // <-- BTW, node.js says - syntax error ; because . follows on the next line
  .on('connection', function (socket) {
    socket.emit('a message', {
        that: 'only'
      , '/chat': 'will get'
    });   // <-- isn't this supposed to be  'a message that only a single socket will get'?

    chat.emit('a message', {
        everyone: 'in'
      , '/chat': 'will get'
    });  // <--, ok, this works as advertised, but the problem - broadcast is not defined for this namespace, so no broadcasting possible
  });

Anyway, thanks for the great socket.io library and I hope someone will make the situation clear about what sending is on a single socket and what works for the entire namsepace.

@3rd-Eden
Copy link
Contributor

Looking in to it, thanks for reporting. @midix you have a different issue than @outsideris so I suggest creating a new issue for that.

@midix
Copy link

midix commented Jun 26, 2011

My problem actually is the same as for @outsideris - currently there is no working way how to broadcast a message on a namespace. I just additonally pointed out that 'broadcast' flag is defined on the 'socket' object and there is no 'broadcast' on the namespace object (where 'broadcast' logically should belong).

@3rd-Eden
Copy link
Contributor

@midix node does not give a syntax error ; because it's on the next line, but because you have a ; after of(chat) and doing a .on after it..

outsideris is having a with the socket.broadcast and that is different than the namespace emit & regular emit that you are talking about. :)

@outsideris
Copy link
Author

I tested more function.
if I use namespce, socket.io functionality are working within namespace. is right?
but room function is now working in namespace too.
if you want, I will share my source.

@dvv
Copy link
Contributor

dvv commented Jun 27, 2011

@midix
Copy link

midix commented Jun 27, 2011

Isn't the socket.emit and namespace.emit the same idea that socket.broadcast.emit() and namespace.broadcast.emit() except that broadcast does not echo message back to the sender? Then the problem is the same: socket.broadcast.emit is sending to no-one (currently socket.emit sends only to itself and broadcasting means "ignore myself", so obviously we have a pretty logical exclusion with the result - send to no-one) and namespace.broadcast.emit gives 'undefined' for 'broadcast', so broadcast is not actually working.

I do not consider the problem that socket.emit sends only to a single client and namsepace.emit is sending to the entire namespace - I thought this behaviour was implemented intentionally but the website example was incorrect.

About having a ; after of(chat) and doing a .on after it - this is what I copied from the socket.io web site namespaces example, so I just pointed out this little error.

Anyway, to avoid further confusion, socket.io website should state more clearly, when we should use socket.emit/send and when namespace.emit/send and which of those two is responsible for (currently broken) broadcasting.

@dvv
Copy link
Contributor

dvv commented Jun 27, 2011

Well, emit and send are similar: emit just preset to deal with JSON and requires manual ack (if ack is ever ordered by setting last argument to be a function).

Client connections are bound to server-side namespaces which you choose by means of manager.of(...) modifier. manager.of('') is equivalent to manager.sockets.

So when you do...

var chat = io.of('/chat').on('connection', function (socket) {
  console.log('...you get a socket bound to "/chat" namespace here');
});

Then if you want to broadcast to "/chat" namespace, you use: socket.broadcast.send/emit. This in fact just puts socket id in exceptions and delegates real action to the corresponding methods of the namespace to which socket belongs.

For that reason, you can do broadcast using namespace methods, which, however, have no knowledge which socket to exclude, and hence broadcast will go to every socket which belongs to the namespace.

@dvv
Copy link
Contributor

dvv commented Jun 27, 2011

For us to quicker fix it, a gist with stripped down server and client code demonstrating the issue (presumably not depending on express-es, jquery-es et al.) is very welcome.

@midix
Copy link

midix commented Jun 27, 2011

@dvv: thanks for the clarification, now I understand why 'broadcast' is defined for the socket and not for the namespace.

Does this mean that

  1. socket.send/emit is equivalent of "send data only to the client, connected to this single socket"
  2. namespace.send/emit is equivalent of "send data to all the clients, whose sockets belong to this namespace"
  3. socket.broadcast.send/emit is equivalent of "send data to all the clients, whose sockets belong to my namespace, but exclude myself"
    ?

If point 1) is true (at least that was how it worked when I tested it 4 days ago), then that example on the socket.io website:

socket.emit('a message', {
that: 'only'
, '/chat': 'will get'
});

should be something more like this:

socket.emit('a message', {
that: 'only myself in'
, '/chat': 'will get'
});

I'll put together and share my minimalistic server/client example with broken broadcasting after 3-4 hours.

@dvv
Copy link
Contributor

dvv commented Jun 27, 2011

@midix: you are right on all points 1-3.

I'm sure you understand that during such major refactoring website can contain both oldies and typos.

Waiting for the example,
--Vladimir

@midix
Copy link

midix commented Jun 27, 2011

Here it is, packed together with the socket.io:

http://filepost.com/files/4mbfbdd1/socket_io_broadcast_issue.tar.gz/
(choose slow download, if asked for a password, leave the field empty)

At first I wanted to check the latest git version from the master branch to see if something has changed, but it gave me 'Object has no method 'Listen'" so I stayed with version that "npm install socket.io" gave me.

The server is a basic triple echo server, which sends the message to all, then back to the sender and then broadcasts it.

The client is basically the same chat html file as with earlier socket.io examples, I just ripped out all unneeded staff and added a namespace. I could run it without problems directly in Chrome and Firefox from a local file.

So when opening test.html in Chrome and Firefox, I could see the messages between browsers sent only once but they should be sent twice because of the 'send' followed by a 'broadcast' on the server. If you comment out the chat.send(msg) line, the messages between both browsers won't work at all - so the broadcast is not sending anything.

@outsideris
Copy link
Author

thanks.
I understand more clearly, because of you.
anyway, so there are bugs with namespace. right?
(sorry my english is poor)

@dvv
Copy link
Contributor

dvv commented Jun 27, 2011

np.
i believe there are bugs, but they are being addressed.

@outsideris
Copy link
Author

of course!! they will.

@Bwen
Copy link

Bwen commented Jul 1, 2011

I also couldnt get the broadcast to work over namespaces either.

The 2 scripts are very simple and standalone, "npm install socket.io" at root and put client in a "http" folder. You can also verify the test for non-namespace socket by just commenting/decommenting the lines "io.of/io.sockets" for server and the connect lines for the client.
Server: http://pastie.org/2149824
Client: http://pastie.org/2149827

The debug clearly shows that the server is "broadcasting packet" for both clients (Webscoket & flashsocket) when they initially connect. But the clients do not fire the event... Do they even get it? I dont know... any debug I could enable on client side to see the traffic at the socket.io level?

Really hope this gets fixed soon.

@Bwen
Copy link

Bwen commented Jul 1, 2011

This fixes the problem:
SocketNamespace.prototype.in = function (room) {
if (this.name === ''){
this.flags.endpoint = '/'+ room;
}
else if (room !== '') {
this.flags.endpoint = this.name +'/'+ room;
}
return this;
};

If the room was empty it was adding an extra slash at the end of the endpoint and for some reason that messed up things.

@gametbt
Copy link

gametbt commented Jul 4, 2011

socket.io v0.7.6: Namespace works on Chrome 12.0.742.91 with websocket and on Firefox 5.0 with flashsocket. If you don't configure the transports option, socket.io defaults to xhr-polling on Firefox and then it does not work.

@Bwen
Copy link

Bwen commented Jul 4, 2011

@gametbt
I'm pretty sure namespaces work in all browers.
its the "broadcast" within them namespace that does not work.
Follow my simplified example.

Still isnt fix in 0.7.6 I had to re-apply my fix locally after an upgrade.

@3rd-Eden
Copy link
Contributor

3rd-Eden commented Jul 4, 2011

@Bwen writing a testcase against it atm so if i can confirm it works I will create patch for it.

@gametbt
Copy link

gametbt commented Jul 5, 2011

@Bwen my statement was in regard to the broadcast to all in the namespace via "emit". I have not tried the 'rooms' yet.

@outsideris
Copy link
Author

I wrote some examples. but it didn't work in my code.

Server : https://github.com/outsideris/socket.io-examples/blob/master/app.js
Client : https://github.com/outsideris/socket.io-examples/blob/master/views/index.jade

there are some examples for test.
Simple Example is work. but Namespace Example and Custom Example is not work.(others are not tested yet.)
If I write code something wrong, let me know...

@gametbt
Copy link

gametbt commented Jul 6, 2011

@outsideris Instead of packing all features in one app, why don't you create separate app for each feature and post those that you have trouble with. Small code fragments will allow you, and anybody that you seek help from, spot the issues easily and quickly.

@outsideris
Copy link
Author

@gametbt I got it. I will write code fregments.

@outsideris
Copy link
Author

I wrote some examples

example 1 : https://gist.github.com/1068731
example 2 : https://gist.github.com/1068737

broadcast.send & broadcast.emit is not work. I don't know why.

express v2.4.1
socket.io v.0.7.6

@3rd-Eden
Copy link
Contributor

3rd-Eden commented Jul 7, 2011

It's already patched in a pull request I made, see referenced commit above, or go directly to pull request: #340

@outsideris
Copy link
Author

@3rd-Eden
It's working nicely on your branch version. really thank you.
I will waiting next version.

@3rd-Eden
Copy link
Contributor

Landed in master

@outsideris
Copy link
Author

thanks to everybody.
this issue makes me more be happy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants