CODEWITHSUNDEEP
javascriptjsonnode.jsrest
Nikolay Dyankov
Nikolay Dyankov

Reputation: 7224

REST API with Node.js failing at simultaneous requests to itself

I made a REST API service using node.js and it works perfectly, until I open a couple of tabs in my browser and make a couple of requests (almost) at the same time. The last tab that sends the request gets the response and the other one hangs.

Basically I have a module called "service" which does all the work:

var service = require('./service');
var server = http.createServer(function(req, res) {
    ...
    var serviceResult = service.runService(parsedURL.query.username, parsedURL.query.api_key);

    res.writeHead(200, {'content-type': 'application/json', 'connection' : 'keep-alive' });

    service.ready = function(serviceResult) {
        var serviceResultJSON = JSON.stringify(serviceResult);
        res.writeHead(200, {'content-type': 'application/json', 'connection' : 'close' });
        res.end(serviceResultJSON);
    }

}

And from the service module I call:

service.ready(result);

... whenever the result is ready and that's my server in a nutshell. So what can I do fix the issue?

EDIT:

Here is what my service.js module looks like (after the suggested changes):

// PUBLIC

exports.runService = function(username, apiKey, callback) {
    _.username              = username;
    _.apiKey                = apiKey;
    init();

    userManager.setLastServiceGlobal(function() {
            // This call triggers the whole cycle. Below is snapshotManager.createdSnapshot(), which gets executed from within snapshotManager and the cycle moves on until apiManager.ready() gets called from within api-manager.js
        snapshotManager.createSnapshot(false);
    });

    // This is the last .ready() function that gets executed when all other modules have finished their job.

    apiManager.ready = function() {
        console.log('API Manager ready.');
        userManager.updateLastService();
        callback(null, serviceResult);
    }
}

// PRIVATE

var userManager             = require('./user-manager'),
    productManager          = require('./product-manager'),
    commentsManager         = require('./comments-manager'),
    apiManager          = require('./api-manager'),
    milestonesManager       = require('./milestones-manager'),
    statisticsManager       = require('./statistics-manager'),
    snapshotManager         = require('./snapshot-manager'),
    serviceResult;

...

snapshotManager.createdSnapshot = function() {
    userManager.refreshUserData();
}
snapshotManager.createdNewSnapshot = function() {
    milestonesManager.generateMilestones();
}
userManager.refreshedUserData = function() {
    userManager.isTimeForPortfolioParse();
}

...

userManager.ready = function() {
    console.log('User Manager ready.');
    userManagerReady = true;
    isDataFetchingOver();
}
productManager.ready = function() {
    console.log('Product Manager ready.');
    productManagerReady = true;
    isDataFetchingOver();
}
commentsManager.ready = function() {
    console.log('Comments Manager ready.');
    commentsManagerReady = true;
    isDataFetchingOver();
}

In this situation the modules are getting overridden just like the "service" module in the "server.js" file, correct? I'm guessing that I need to implement callbacks everywhere instead of the .ready() functions, correct?

Upvotes: 0

Views: 1742

Answers (2)

deoxxa
deoxxa

Reputation: 728

There are three problems here!

  1. You're setting the ready property on service, while there's only one service object. If two requests happen very near to each other, you might end up overwriting the ready property with the second request before it's been fired for the first request. This is because there's only one instance of the service object. That's almost definitely the way it should be, so don't worry about that, but rather we have to find a new way to signal to your consuming code that the service action has completed.
  2. Your res variable inside service.read is probably not the same res that you think it is. The second time it gets called, it'll be different. This is because you're redefining the ready property, so it's getting a different scope every time.
  3. You're sending the headers twice. That's an easy fix - just don't send any headers until you know what they should be. That'll become obvious just below.

To fix the first problem, I suggest that you use callbacks! You should be familiar with them already from the node core, they're used for just about everything. This also happens to fix the second problem, by virtue of scope in JavaScript.

Note: the reason I suggest using callbacks instead of a factory or some other method is that callbacks are pervasive in node-land, so if you use them, there's a good chance you'll have a much easier time integrating with other libraries and such.

Here's how I'd do your server code:

var service = require('./service');
var server = http.createServer(function(req, res) {
  // note that we're not immediately sending headers anymore - this
  // fixes the third problem with what you had before

  service.runService(parsedURL.query.username, parsedURL.query.api_key, function(err, serviceResult) {
    // check for errors! tell the client!
    if (err) {
      res.writeHead(500);
      res.end();
      return;
    }

    res.writeHead(200, {
      'content-type': 'application/json',
      'connection' : 'keep-alive',
    });

    var serviceResultJSON = JSON.stringify(serviceResult);

    res.end(serviceResultJSON);
  });
};

And here's how I'd implement the service thing:

var service = module.exports = {};

service.runService = function runService(username, key, cb) {
  // assume `database` exists from somewhere else...
  database.getUser(username, function(err, user) {
    // make sure we send any errors up the line
    if (err) {
      cb(err);
      return;
    }

    // here's an error we've decided on
    if (user.key !== key) {
      cb(Error("key is incorrect!"));
      return;
    }

    // this is a very common usage pattern - cb(error, result, ...)
    // the reason we're calling this will `null` for the error is a bit
    // of a double negative - we're saying "there was no error".
    cb(null, user);
  });
};

Upvotes: 3

Raul Martins
Raul Martins

Reputation: 405

The problem is they all share the same service object. So what happens is that when you receive a request you overwrite service.ready to a function that responds to that request. So if you get 2 requests really close together, service.ready will be set to respond to the last request you got and so, only that one will get a response.

The best way is to have the service module export a function that returns an instance of service like this:

 function serviceFactory() {

  }  

 serviceFactory.prototype.getService() {
    return new Service();
 }
 module.exports = serviceFactory;

And then you can have

var serviceFactory = require(./service);
var server = http.createServer(function(req, res) {
 ...
 var service = serviceFactory.getService();
 var serviceResult = service.runService(parsedURL.query.username, parsedURL.query.api_key);

 res.writeHead(200, {'content-type': 'application/json', 'connection' : 'keep-alive' }); 

 service.ready = function(serviceResult) {
     var serviceResultJSON = JSON.stringify(serviceResult);
     res.writeHead(200, {'content-type': 'application/json', 'connection' : 'close' });
     res.end(serviceResultJSON);
 }
}

Upvotes: 0

Related Questions