Nodejs Event gets emitted multiple times

Question

export function postComment(req, res) {

const user = decodeToken(req);

let saveComment, saveJob, saveUser, activity, jobUser;

function pushToJob(comment) {
    saveComment = comment;
    Job.findById(req.params.id)
        .then((data) => {
            job = data;
            data.comments.push(saveComment._id)
            return data.save()
        }).catch((err) => {
            throw new Error(`The error at pushing to job is ${err}`)
        })
}

function updateCommentWithJobId(job) {
    saveComment.jobId = {
        _id: job._id,
        title: job.title
    }
    return saveComment.save()
}

function addActivityToUser(comment) {
    saveComment = comment;
    User.findById(user._id)
        .then((data) => {
            saveUser = data;
            activity = {
                activity: 'comment',
                comment: saveComment._id,
                jobAuthor: {
                    _id: saveJob.userId._id,
                    name: saveJob.userId.name
                }
            }
            saveUser.activities.unshift(activity);
            return saveUser.save()
        }).catch((err) => {
            throw new Error(`The error at addActivityToUser is ${err}`)
        })
}

function addUserToComment(user) {
    saveUser = user;
    saveComment.userId = {
        _id: saveUser._id,
        name: saveUser.name,
        profile_image: saveUser.profile_image
    }

    return saveComment.save()
}

function addActivityToJobUser(comment) {
    saveComment = comment
    User.findById(saveJob.userId)
        .then((data) => {
            if (saveJob.userId !== user._id) {
                data.activities.unshift(activity);
            }

            return data.save()
        }).catch((err) => {
            throw new Error(`The error at addActivityToJobUser ${err}`)
        })
}

function emitCommentEvent(user) {
    jobUser = user
    let comment = {
        userId: saveJob.userId,
        room: saveJob.room,
        comment: saveComment
    }

    comEmit.emit('comment', comment);
    return res.status(200).json({ message: 'Comment posted successfully' });
}

Comment.create(req.body)
    .then(pushToJob)
    .then(updateCommentWithJobId)
    .then(addActivityToUser)
    .then(addUserToComment)
    .then(addActivityToJobUser)
    .then(emitCommentEvent)
    .catch((err) => {
        res.status(500).json({ message: 'An error occurred while posting your comment, please try again' })
    })
}

This is controller for one of my api, and I am emitting the comment event every time someone posts a comment on an author's post. The problem is that the same event gets fired multiple times. The line console.log('about to emit comment') gets triggered just once.

I tried looking for a solution, but didn't find any answer yet.

What could be the cause for such behavior?

EDIT: Here's the listener for the event.

  socketio.on('connection', function(socket){
      Chat.find({})
          .then((data)=>{

                //Some independent socket events

                comEmit.on('comment', (data) => {
                    console.log('comment posted ', data)
                    if (userId === data.userId) {
                        socket.join(data.room);
                    }
                    socket.in(data.room).emit('commentPosted', data.comment)
                })
           })
   })

As I can check the logs, 'comment posted' gets logged multiple times and the data is the same.

It is very hard to understand what are you trying to do, you drastically need to refactor your code, this is just promise hell. — Alexandru Olaru, Aug 25 '17 at 12:57
Often times, the problem is at the other end where you accidentally registered more than one listener so you think you're getting multiple events, but actually you have multiple listeners that are each firing on only one event. Can you show us where you register the listener for the `comment` event? I would guess that the listener is perhaps inside a request handler that gets called more than once so each time its called, it adds another listener. — jfriend00, Aug 25 '17 at 15:14
agree with @ alexabdru - you can use promises without exclusively relying on anonymous functions. And I am not completely sure that nesting is necessary. To your problem either add GUID or ITMESTAMP to the emitted message to help determine if it is in fact emitted once OR multiple times. that can give you insight as to where to investigate as @ jfriend00 says it might be on the consumption side. — akaphenom, Aug 25 '17 at 15:23
This code should be chained at the top level, not nested. Please learn how to use promise chaining instead of nesting. Also, it appears you have chosen a coding style that spreads the code out over the maximum number of lines and levels of indent making it very difficult to follow. — jfriend00, Aug 25 '17 at 15:34
@jfriend00 that is my fault. I am trying to refactor the code, to make it readable. — relentless-coder, Aug 25 '17 at 15:43
Note that your current code also does not propagate back to the caller most errors. If any of your promises fail, the nesting just stops and none of the errors except the very first promise will trigger your `.catch()` handler. So, this code silently eats most errors. — jfriend00, Aug 25 '17 at 15:48
Start here with [these articles](https://www.google.com/search?q=promise+chain+vs.+nest). Also, your code may benefit from [How to chain and share prior results](https://stackoverflow.com/questions/28714298/how-to-chain-and-share-prior-results-with-promises/28714863#28714863). Also, to enable proper error propagation and error handling, you have to return any nested promise in their parent `.then()` handler and you can't use traditional callbacks in the middle of the promise chain (they have to be converted to promises too). — jfriend00, Aug 25 '17 at 16:00
And, don't hesitate to break chunks of code into their own function to make things a lot simpler to follow and understand. And, I've never understood the coding style that puts `.then()` on a new line detached from the promise it belongs to. IMO, it just makes the code more complicated to read and follow and introduces more indentation than the logic actually has. — jfriend00, Aug 25 '17 at 16:05
And, you should pretty much never have a promise inside a `.then()` handler that isn't returned because that's an orphaned promise chain who's error is never propagated and will even generate a warning in some more modern environments (because it's almost always wrong). — jfriend00, Aug 25 '17 at 16:06
This should be helpful also: https://stackoverflow.com/questions/23803743/what-is-the-explicit-promise-construction-antipattern-and-how-do-i-avoid-it — agm1984, Aug 25 '17 at 17:02
I didn't study the code, but generally looks easier to follow. If you still have the same problem, then please respond to my first comment here on your question as that's still my best guess for where the original problem is. — jfriend00, Aug 25 '17 at 17:24
Yes, I deleted that comment a few minutes ago because it was wrong. What I really want to see is where you register the event handler for the `comment` message that is duplicated. That's likely where the problem is as explain in my first comment to your question. — jfriend00, Aug 25 '17 at 17:36
We need to see what the listener is inside of and when that gets called. Is that inside a request handler? If so, then you are creating a duplicate event listener every time that request handler is called. And, thus you think the event is occurring multiple times, but it's really just that you have multiple duplicate listeners for the same event. — jfriend00, Aug 25 '17 at 17:41
@jfriend00 I can't add the complete code, since it's chat app configuration so it's quite large, but I have pasted some code that would give you an idea of the situation, maybe. — relentless-coder, Aug 25 '17 at 17:48
So, it appears that you call `comEmit.on('comment', ...)` for EVERY new socket connection. Unless this is a different `comEmit` for each socket, that creates duplicate event handlers. Do you see that? And, do you remove the event listeners when the socket disconnects too? — jfriend00, Aug 25 '17 at 17:50
@jfriend00 yes, I think that's the issue. Because the `comEmit` is just one, for every socket. I'll think of a workaround, and if I can't figure this out, then i'll ask for help. Thanks for helping so far, and also for providing with the resources to learn chaining. — relentless-coder, Aug 25 '17 at 17:54

score 5 · Answer 1 · answered Aug 25 '17 at 18:00

I'll make my comments into an answer.

In your socketio.on('connection', ...) handler, you are doing:

comEmit.on('comment', (data) => { ...});

Since there is only one comEmit object, that means that each incoming socket.io connection causes you to add new duplicate event handler for the comment event. So, you aren't actually getting duplicate events like you thought, but have duplicate event handlers. So, when the comment event is triggered, you get multiple, duplicate event handlers for that event all running.

The solution is usually to add the event handler statically, just once when you set up the object outside of any request handler so you don't get duplicates, but exactly how to do that depends upon the overall code context and what you're trying to do which I don't quite follow from the limited code we've seen.

The premise is that, when a user posts a comment on a post, everyone who is a part of the `room` gets notified. So, someone posts a comment, `comment` event is fired, and then inside event handler's callback, `socketio` fires an event in that particular room. — relentless-coder, Aug 25 '17 at 18:08
i'll try and pull the event handler out of `socketio.on('connection', ...)` handler, the only obstruction would be `socket.join(room)` inside the `comEmit.on('comment', ....)` handler. Thanks for clearing things up. — relentless-coder, Aug 25 '17 at 18:13

Nodejs Event gets emitted multiple times

1 Answers1