Unexpected multiple emits (multiple rooms joined)

[bbars]

https://github.com/googollee/go-socket.io/blob/101f99ce012b002ac8b85d901399054e2227d76b/broadcast.go#L105-L119

According to this code, if some socket is a member of multiple rooms, then it receives message multiple times. I think it's a bug.

[erkie]

Should be a fairly easy fix, to use a map instead of just plainly emitting. Note I've released the lock earlier, which should be better for performance? What do you think?

// SendAll sends given event & args to all the connections to all the rooms
func (broadcast *broadcast) SendAll(event string, args ...interface{}) {
	// get a read lock
	broadcast.lock.RLock()

	conns := make(map[string]Conn)
	// iterate through each room
	for _, connections := range broadcast.rooms {
		// iterate through each connection in the room
		for _, connection := range connections {
			// emit the event to the connection
			conns[connection.ID()] = connection
		}
	}
	broadcast.lock.RUnlock()

	for _, connection := range conns {
		connection.Emit(event, args...)
	}
}

[bbars]

I think, it is too hard to make a new map each time we want to broadcast. Instead I propose to join each socket to default room automatically (for example, "*"). Therefore we can broadcast to that room. I've already fixed it within fork: join default room, broadcast.

[erkie]

I think it's an interesting tradeoff, making a map per broadcast might be slow vs the extra memory of keeping every connection in a default room. I'd actually vote for the map approach, since CPU is cheaper than memory.

[bbars]

When you make a map per broadcast, you're using memory too (the same size). But there would be another memory allocation each time you want to broadcast. Also there are 3 loops instead of 1.

[erkie]

Then it's a question of how much memory should we allocate for this scenario. Our use case for sockets does not require it, so it will add extra overhead that is not necessary for our case. Using a map or data structure to check uniqeuness while broadcasting might be a more worthwhile approach.

[adrianmxb]

I am not really a fan of the default room solution either. This library is far from the standard in some regards already and it will get harder and harder to ensure compatibility if we continue to brew our own beer and not adhere to the spec.

Short input on how official library handles it: https://github.com/socketio/socket.io-adapter/blob/ae23c7ef4cd1b1793121b5af0376686a2ba2deea/index.js#L123-L148

[erkie]

I was looking at that just yesterday as well! I think it's a good solution to just use the map to see who has been "emit"ed in the loop. I wouldn't worry about the overhead of the map in the case at all.

Another thing I noticed in the socket.io-adapter implementation is that broadcasting is not blocking. While it is in our implementation:

https://github.com/googollee/go-socket.io/blob/master/broadcast.go#L101

Could solve a bunch of issues by adding go connection.Emit(event, args...) on that line. (I'm trying it out in production, and it's looking good so far)