Bug fix in chat application

I received a bug report from someone who downloaded and installed my node/socket.io chat app, which I’ve posted about a few times before. The bug showed up in a very specific scenario, and it took some head-scratching to track down:

1. user1 joins the server and creates a room
2. user2 joins the server and joins user1’s room
3. user1 disconnects from the server, which destroys the room and kicks everyone out
4. user2 was still able to send messages, which crashed the server

I’m sharing the fix here because others may run into the same thing.

The code is structured so that a people object stores information about every connected user. Here’s an example:

{
Pdd0QA1bYnH2nJZQPT7c:
    { name: 'Dave',
      owns: '905c3f49-d7d1-4403-aa8a-20d71f92600c',
      inroom: '905c3f49-d7d1-4403-aa8a-20d71f92600c',
      device: 'desktop',
      country: 'gb' },
RhszumhtYGTpy5uwPT7d:
  { name: 'Adam',
    owns: null,
    inroom: '905c3f49-d7d1-4403-aa8a-20d71f92600c',
    device: 'desktop',
    country: 'it' }
}

The keys are socket identifiers (socket.id). From this data structure a few things are clear:

• The owns property reveals who owns a room. (The ID is a node-uuid generated value created at room creation.)
• The inroom property shows which room a user has joined. Comparing owns and inroom reveals ownership.
• If inroom has a value but owns is empty, that user isn’t a room owner.
• In the example above, Dave owns the room and Adam has joined it.

There’s also a rooms collection:

{
  '7fe96889-e898-4935-b87f-22d5c0338b0e':
    { name: 'FormulaOne',
      id: '7fe96889-e898-4935-b87f-22d5c0338b0e',
      owner: 'KWusKywHePS1JVdtRqKd',
      people: [ 'KWusKywHePS1JVdtRqKd', 'LlDMvpHsjHnLkyknRqKe' ],
      peopleLimit: 4,
      status: 'available',
      private: false }
  }

The main key is the randomly generated node-uuid. The owner’s ID and all connected people are stored as unique socket.id values. (The peopleLimit, status and private variables aren’t used yet; they’re earmarked for future enhancements.)

The logic for removing users when a room is destroyed gets complicated. A room can only be removed by its owner. If the owner disconnects from the server, leaves the room, or removes the room, different actions need to fire. When a room owner disconnects, the following has to happen:

• Notify all connected users that the room’s owner has left and they’ll be disconnected
• Remove the user from the people collection
• Delete the room from the room collection
• Delete the chat history belonging to the room
• Disconnect all users from the room

The last point is the tricky one. The inroom property for connected users must be set to null, otherwise they can’t join another room. Why? Because the joinRoom method runs this check:

if (people[socket.id].inroom !== null) {
  socket.emit(
    'update',
    'You are already in a room (' +
      rooms[people[socket.id].inroom].name +
      '), please leave it first to join another room.'
  );
}

But that wasn’t enough. Setting the properties to null let users join other rooms from my code’s perspective, but from socket.io’s perspective, things were still broken.

On room creation and join, I use socket.io’s Room functionality.

Rooms allow simple partitioning of the connected clients. This allows events to be emitted to subsets of the connected client list, and gives a simple method of managing them.

Here’s how that works:

socket.room = room.name;
socket.join(socket.room);

This restricts emitted messages to people in the room.

Now we’re getting to the actual bug.

The error on the server was (trimmed):

chatHistory[socket.room].push(people[soc
ket.id].name + ": " + msg);
TypeError: Cannot call method 'push' of undefined

This push call fires in the send method every time a user in a room sends a message. The last 10 messages are stored in the chatHistory array.

It fails because chatHistory[socket.room] is undefined. All the objects belonging to the room were wiped out when the owner disconnected.

The problem lives in this block inside the send function:

if (
  io.sockets.manager.roomClients[socket.id]['/' + socket.room] !== undefined
) {
  io.sockets.in(socket.room).emit('chat', people[socket.id], msg);
  socket.emit('isTyping', false);
  if (_.size(chatHistory[socket.room]) > 10) {
    chatHistory[socket.room].splice(0, 1);
  } else {
    chatHistory[socket.room].push(people[socket.id].name + ': ' + msg);
  }
} else {
  socket.emit('update', 'Please connect to a room.');
}

When user1 creates a room, user2 joins, then user1 disconnects, user2 should be both removed from the server and disconnected from the socket.io room. That disconnection was failing, so the first if statement above evaluated to true, and the subsequent code tried to reference values that had already been destroyed.

Adding this log statement to the send function confirmed it:

console.log(io.sockets.manager.roomClients[socket.id]);

Even though the room no longer existed, the user was still joined to it.

The fix requires two steps. First, loop through all connected sockets (stored in a sockets collection) and check if their IDs match any in the room.people array. If there’s a match, force that socket to leave the room.

Second, set the inroom property to null for users who were in that room.

var socketids = [];
for (var i = 0; i < sockets.length; i++) {
  socketids.push(sockets[i].id);
  if ((_.contains(socketids), room.people)) {
    sockets[i].leave(room.name);
  }
}

if ((_.contains(room.people), s.id)) {
  for (var i = 0; i < room.people.length; i++) {
    people[room.people[i]].inroom = null;
  }
}

That solved it. An interesting challenge. I also pulled the above code into a function since it was needed in three different places, which tidied up the logic considerably. The latest changes are on GitHub.