res.send() is not sending current response, instead keeps last one

Question

This is some of my code that I have in my index.js. Its waiting for the person to visit url.com/proxy and then it loads up my proxy page, which is really just a form which sends back an email and a code. From my MongoDB database, I grab the users order using the code, which contains some information I need (like product and the message they're trying to get). For some reason, it seems like its responding before it gets this information and then holds onto it for the next time the form is submitted.

The newline in my res.send(product + '\n' + message) isnt working either, but thats not a big deal right now.

But.. for example, the first time I fill out the form ill get a blank response. The second time, I'll get the response to whatever I filled in for the first form, and then the third time ill get the second response. I'm fairly new to Web Development, and feel like I'm doing something obviously wrong but can't seem to figure it out. Any help would be appreciated, thank you.

   app.get('/proxy', function(req,res){
        res.sendFile(__dirname+ "/views/proxy.html");
    });

    var message = "";
    var product = "";

    app.post('/getMessage', function(req,res)
    {
        returnMsg(req.body.user.code, req.body.user.email);
        //res.setHeader('Content-Type', 'text/plain');
        res.send(product + "\n" + message);
    });

    function returnMsg(code, email){
        MongoClient.connect(url, function(err, db){
            var cursor = db.collection('Orders').find( { "order_id" : Number(code) })
            cursor.each(function(err, doc){
                assert.equal(err, null);
                if (doc!= null)
                {
                        message = doc["message"];
                        product = doc["product"];
                }
                else {
                        console.log("wtf");
                    // error code here
                }
            });
            console.log(email + " + " + message);
            var document = {
                    "Email" : email,
                    "Message" : message
            }
            db.collection("Users").insertOne(document);
            db.close();
        });
    }

Answer 1

You need to do lots of reading about your asynchronous programming works in node.js. There are significant design problems with this code:

1. You are using module level variables instead of request-level variables.
2. You are not correctly handling asynchronous responses.

All of this makes a server that simply does not work correctly. You've found one of the problems already. Your async response finishes AFTER you send your response so you end up sending the previously saved response not the current one. In addition, if multiple users are using your server, their responses will tromp on each other.

The core design principle here is first that you need to learn how to program with asynchronous operations. Any function that uses an asynchronous respons and wants to return that value back to the caller needs to accept a callback and deliver the async value via the callback or return a promise and return the value via a resolved promise. The caller then needs to use that callback or promise to fetch the async value when it is available and only send the response then.

In addition, all data associated with a request needs to stay "inside" the request handle or the request object - not in any module level or global variables. That keeps the request from one user from interfering with the requests from another user.

To understand how to return a value from a function with an asynchronous operation in it, see How do I return the response from an asynchronous call?.

---

What ends up happening in your code is this sequence of events:

1. Incoming request for /getMessage
2. You call returnMsg()
3. returnMsg initiates a connection to the database and then returns
4. Your request handler calls res.send() with whatever was previously in the message and product variables.
5. Then, sometime later, the database connect finishes and you call db.collection().find() and then iterate the cursor.
   6/ Some time later, the cursor iteration has the first result which you put into your message and product variables (where those values sit until the next request comes in).

In working out how your code should actually work, there are some things about your logic that are unclear. You are assigning message and product inside of cursor.each(). Since cursor.each() is a loop that can run many iterations, which value of message and product do you actually want to use in the res.send()?

---

Assuming you want the last message and product value from your cursor.each() loop, you could do this:

app.post('/getMessage', function(req, res) {
    returnMsg(req.body.user.code, req.body.user.email, function(err, message, product) {
        if (err) {
            // send some meaningful error response
            res.status(500).end();
        } else {
            res.send(product + "\n" + message);
        }
    });
});

function returnMsg(code, email, callback) {
    let callbackCalled = false;
    MongoClient.connect(url, function(err, db) {
        if (err) {
            return callback(err);
        }
        var cursor = db.collection('Orders').find({
            "order_id": Number(code)
        });
        var message = "";
        var product = "";

        cursor.each(function(err, doc) {
            if (err) {
                if (!callbackCalled) {
                    callback(err);
                    callbackCalled = true;
                }
            } else {
                if (doc != null) {
                    message = doc["message"];
                    product = doc["product"];
                } else {
                    console.log("wtf");
                    // error code here
                }
            }
        });
        if (message) {
            console.log(email + " + " + message);
            var document = {
                "Email": email,
                "Message": message
            }
            db.collection("Users").insertOne(document);
        }
        db.close();
        if (!callbackCalled) {
            callback(null, message, product);
        }
    });
}

Personally, I would use promises and use the promise interface in your database rather than callbacks.

---

This code is still just conceptual because it has other issues you need to deal with such as:

1. Proper error handling is still largely unfinished.
2. You aren't actually waiting for things like the insert.One() to finish before proceeding.