Mongoose异步回调问题：findUserById提前返回致数据重复

Hey there, let's break down what's going wrong here and fix it step by step:

The Core Issues #

1. Closure Problem with var: You're using var to declare variables like userId, body inside your for loop. Since var is function-scoped, all your callback functions end up referencing the same variable instance. By the time the async User.findById calls resolve, the loop has already finished running, so all callbacks use the last value from the loop.
2. Unreliable Async Control: Using a fixed setTimeout to wait for async operations is a hack—you can't guarantee all database calls will finish in 10ms. This will lead to race conditions where some user data isn't loaded when you send the response.
3. Redundant Function Declarations: Defining findUsersById and myCallBack inside the loop creates a new function on every iteration, which is inefficient and adds unnecessary complexity.

The Fix: Use Promises + Async/Await #

Modern Node.js (and MongoDB's Mongoose, which I assume you're using here) supports Promises and async/await, which make handling asynchronous code way cleaner and avoid these kinds of bugs. Here's how to refactor your code:

router.post('/messages', async (req, res) => { 
  try {
    const users = req.body.users.sort(); 
    const room = await Room.findOne({ users: users });

    if (!room) {
      return res.status(400).send(errorResponse(null, "Message Not Found.")); 
    }

    const messages = await Message.find({ roomId: room._id });
    if (messages.length === 0) {
      return res.send(successResponse({ messages: [], users }, "No messages found."));
    }

    // Map each message to a promise that fetches the user and formats the message
    const alteredMessagesPromises = messages.map(async (message) => {
      const user = await User.findById(message.userId);
      if (!user) {
        console.log(`User with ID ${message.userId} not found`);
        // Handle missing user case—maybe return the message without userName?
        return {
          userId: message.userId,
          userName: 'Unknown User',
          body: message.body,
          createdAt: message.createdAt,
          updatedAt: message.updatedAt
        };
      }
      return {
        userId: message.userId,
        userName: user.name,
        body: message.body,
        createdAt: message.createdAt,
        updatedAt: message.updatedAt
      };
    });

    // Wait for all promises to resolve
    const alteredMessages = await Promise.all(alteredMessagesPromises);
    const messageResponse = { messages: alteredMessages, users: users };

    console.log("Sending response");
    console.log(messageResponse);
    res.send(successResponse(messageResponse, "Messages Successfully Found."));
  } catch (err) {
    // Handle any errors that occur during the async operations
    console.error(err);
    res.status(500).send(errorResponse(null, "An error occurred while fetching messages."));
  }
});

Key Improvements: #

• Async/Await: Makes the code read like synchronous code, eliminating callback hell and making it easier to follow.
• let/const: Using const (and let where needed) instead of var ensures block scoping, so each iteration of map has its own reference to the message object.
• Promise.all: Properly waits for all user lookup promises to complete before sending the response—no more guesswork with setTimeout.
• Error Handling: Wrapped everything in a try/catch block to handle any unexpected errors (like database connection issues) gracefully.
• Cleaner Structure: Removed redundant nested functions and flattened the code for better readability.

Bonus: Optimize Database Queries #

If you have a lot of messages, fetching users one by one can be slow. You can optimize this by fetching all unique user IDs at once:

// Get all unique user IDs from messages
const userIds = [...new Set(messages.map(msg => msg.userId))];
// Fetch all users in a single query
const usersMap = new Map();
const users = await User.find({ _id: { $in: userIds } });
users.forEach(user => usersMap.set(user._id.toString(), user.name));

// Now map messages to alteredMessages without additional queries
const alteredMessages = messages.map(message => {
  const userName = usersMap.get(message.userId.toString()) || 'Unknown User';
  return {
    userId: message.userId,
    userName,
    body: message.body,
    createdAt: message.createdAt,
    updatedAt: message.updatedAt
  };
});

This reduces the number of database calls from N (number of messages) to 1, which is much more efficient for large datasets.

内容的提问来源于stack exchange，提问作者ManjulSigdel