5
var express = require('express');
var app = express();
var server = app.listen(3000);
var replyFromBot;
app.use(express.static('public'));
var socket = require('socket.io');
var io = socket(server);
io.sockets.on('connection' , newConnection);

function newConnection(socket) {
   console.log(socket.id);
   listen = true;
   socket.on('Quest' ,reply);
   function reply(data) {
     replyFromBot = bot.reply("local-user", data);
     console.log(socket.id+ " "+replyFromBot);
     socket.emit('Ans' , replyFromBot);
  }
}

i've created a server based chat-bot application using node.js socket.io and express but the thing is for first time when i call socket.on it gets executed once and for 2nd time it gets executed twice for 3rd thrice and so on i've tackled this issue by setting a flag on my client so that it would display only once. i just wants to know is my code logically correct i mean is this a good code? because if the client ask a question for 10th time than listeners array will have 10+9+8....+1 listeners it would go on increasing depending upon number of questions clients asked. which is not good

i tried using removeListener it just removes listener once and it dosent call back for 2nd time. what do you guys recommend? do i go with this or is there any other way to add the listener when socket.on called and remove it when it gets executed and again add listener for the next time it gets called

thank-you.

client code:

function reply() {
  socket.emit('Quest' , Quest);
  flag = true;
  audio.play();
  socket.on('Ans', function(replyFromBot) {
  if(flag) {
    console.log("hi");
    var para = document.createElement("p2");
    x = document.getElementById("MiddleBox");
    para.appendChild(document.createTextNode(replyFromBot));
    x.appendChild(para);
    x.scrollTop = x.scrollHeight;
    flag = false;
  }
 });
}
Prem
  • 71
  • 1
  • 1
  • 9
  • It's not clear exactly what design you're trying to achieve so I don't really understand what the problem is or what you expect the correct behavior to be. Your code also appears to use some shared variables like `replyFromBot` and `listen` which are going to be shared among multiple user's requests and will likely cause problems. But, I don't see any problems in this specific code that would cause duplicate event handlers. Are there duplicate handlers on the client side? – jfriend00 Apr 12 '18 at 05:30
  • replyFromBot and listen are just things used in another function they wont cause any issue ignore them. – Prem Apr 12 '18 at 05:32
  • what i m trying to achieve is user sends question to a server question is like "data = Hi can you help me with this" , socket.emit('Quest' , data); bot.reply will search for appropriate answer to the question , and will reply to the client in form of socket.emit('Ans' , replyFromBot); – Prem Apr 12 '18 at 05:35
  • so for first time if question is asked then ill get one reply but then if i ask it for second time ill get same reply 2 times for third question ill get 3 replies and so on – Prem Apr 12 '18 at 05:36
  • Your server code as you show it here does not cause that problem. You have one and only one server event handler per socket for the `'Quest'` message and you send one `'Ans'` response each time you get an incoming message. I don't see how the code you show causes the problem you describe unless the problem is caused on the client side. Perhaps we need to see the client code. – jfriend00 Apr 12 '18 at 05:49
  • edited the question added client code used a flag so that i wont get multiple output it will display reply once but if i remove it will display multiple times for first question hi will be printed once , for 2nd it will print twice for third thrice. – Prem Apr 12 '18 at 06:00

2 Answers2

17

The problem is caused by your client code. Each time you call the reply() function in the client you set up an additional socket.on('Ans', ...) event handler which means they accumulate. You can change that to socket.once() and it will remove itself each time after it get the Ans message. You can then also remove your flag variable.

function reply() {
  socket.emit('Quest' , Quest);
  audio.play();
  // change this to .once()
  socket.once('Ans', function(replyFromBot) {
    console.log("hi");
    var para = document.createElement("p2");
    x = document.getElementById("MiddleBox");
    para.appendChild(document.createTextNode(replyFromBot));
    x.appendChild(para);
    x.scrollTop = x.scrollHeight;
  });
}

Socket.io is not really built as a request/response system which is what you are trying to use it as. An even better way to implement this would be to use the ack capability that socket.io has so you can get a direct response back to your Quest message you send.

You also need to fix your shared variables replyFromBot and listen on your server because those are concurrency problems waiting to happen as soon as you have multiple users using your server.


Better Solution

A better solution would be to use the ack capability that socket.io has to get a direct response to a message you sent. To do that, you'd change your server to this:

function newConnection(socket) {
    console.log(socket.id);
    socket.on('Quest', function(data, fn) {
      let replyFromBot = bot.reply("local-user", data);
      console.log(socket.id+ " "+replyFromBot);
      // send ack response
      fn(replyFromBot);
    });
}

And, change your client code to this:

function reply() {
    audio.play();
    socket.emit('Quest', Quest, function(replyFromBot) {
        console.log("hi");
        var para = document.createElement("p2");
        x = document.getElementById("MiddleBox");
        para.appendChild(document.createTextNode(replyFromBot));
        x.appendChild(para);
        x.scrollTop = x.scrollHeight;
    });
}

Doing it this way, you're hooking into a direct reply from the message so it works as request/response much better than the way you were doing it.

jfriend00
  • 683,504
  • 96
  • 985
  • 979
  • I added a code example for using the `ack` feature that socket.io has to get a direct response back from a message. – jfriend00 Apr 12 '18 at 06:16
  • thanks for the ack code also this is exactly what i wanted. and yea i realized about those shared variables ill look into that. – Prem Apr 12 '18 at 09:00
1

Instead of socket.on('Quest' ,reply); try socket.once('Quest' ,reply);

The bug in your code is that each time newConnection() is called node registers a event listener 'Quest'. So first time newConnection() is called the number of event listener with event 'Quest' is one, the second time function is called, number of event listener increases to two and so on

socket.once() ensures that number of event listener bound to socket with event 'Quest' registered is exactly one

mnille
  • 1,328
  • 4
  • 16
  • 20
Nitin
  • 21
  • 3
  • socket.once will just call once i mean it will work for question asked for first time and second time client ask question socket.once wont execute i tried this it wont work .i want to work it in like a loop as client will ask questions again. thanks thought – Prem Apr 12 '18 at 05:46
  • This does not seem to me like the problem. The `connection` event only occurs once per connection so there will only be one event handler for the `'Quest'` message for each socket. – jfriend00 Apr 12 '18 at 05:50