TLS support breaks cluster mode

[juliangut]

I've been using this library for long on its version 0.2.0 but just recently I've had the need to implement an SMPP server with TLS and so updated to 0.3.1

A single instance of the server (with and without TLS) runs smoothly but to my despair it is not the case when you run several instances with PM2 on cluster mode, non-TLS instances get broken effectively making the library usable only for TLS servers when on cluste mode

TypeError: Cannot read property 'context' of undefined
    at Server.getTicketKeys (_tls_wrap.js:915:28)
    at Server._getServerData (_tls_wrap.js:904:22)
    at EventEmitter.cluster._getServer (internal/cluster/child.js:79:24)
    at listenInCluster (net.js:1409:11)
    at Server.listen (net.js:1480:7)
    at Server.listen (/home/[...]/node_modules/smpp/lib/smpp.js:176:37)
    at Object.<anonymous> (/home/[...]/server.js:234:8)
    at Module._compile (module.js:652:30)
    at Object.Module._extensions..js (module.js:663:10)
    at Module.load (module.js:565:32)

I assume this functionality reduction was not meant when TLS support was added on version 0.3.0, I guess no one else has tried running several SMPP servers with and without TLS on the same machine

apps:
  - name: server
    script: ./server.js
    instances: 4
    autorestart: true
    max_restarts: 3
    min_uptime: 30000
    error_file: ~/.pm2/log/server-err.log
    out_file: ~/.pm2/log/server-out.log
    exec_mode: cluster
    kill_timeout: 5000
  - name: server-tls
    script: ./server.js
    args: tls
    instances: 2
    autorestart: true
    max_restarts: 3
    min_uptime: 30000
    error_file: ~/.pm2/log/server-tls-err.log
    out_file: ~/.pm2/log/server-tls-out.log
    exec_mode: cluster
    kill_timeout: 5000

The problem seems to be the inheritance of the Server object from tls.Server, so changing this line solves the situation while keeping support for TLS

util.inherits(Server, tls.Server);
//to
util.inherits(Server, net.Server);

Additionally always listening from tls doesn't look correct to me when you can decide transport the same way you do on Server creation:

Server.prototype.listen = function() {
    var transport = this.tls ? tls : net;
    var args = [this.tls ? 3550 : 2775];
    if (typeof arguments[0] == 'function') {
        args[1] = arguments[0];
    } else if (arguments.length > 0) {
        args = arguments;
    }
    return transport.Server.prototype.listen.apply(this, args);
};

I can make a simple PR to fix this, I need this patch ASAP so I'll do it only if I can be assured it will be accepted and merged quickly, as the project really seems dead, otherwise I'll be forced to fork the project

[juliangut]

A possible, absolutely inelegant, solution can be to extend Server from net in case you don't want it to use TLS:

var net = require('net');
var util = require('util');
var smpp = require('smpp');

var serverOptions = {};
var socketPort = 2775;

if (/* TLS enabled server */) {
    serverOptions = {
        cert: fs.readFileSync(/*certFile*/),
        key: fs.readFileSync(/*keyFile*/)
    };
    socketPort = 3550;
} else {
    util.inherits(smpp.Server, net.Server);
}

var server = smpp.createServer(serverOptions, function(session) {
    //....
});

server.listen(socketPort);

In this way both TLS and non-TLS sockets and sessions are created correctly

[farhadi]

@juliangut I'm not sure if we can inherit from net.Server for both tls and non-tls mode. If you can confirm that it works in both modes please make a PR of your changes, so I can merge it.

[juliangut]

Hi @farhadi, I'm afraid inheriting from net.Server doesn't work for TLS servers, right now I'm testing the workaround I posted earlier and it looks good so far

I'm not sure if it can be integrated into the library's Server constructor but I believe it can:

function Server(options, listener) {
    var self = this;
    this.sessions = [];

    if (typeof options == 'function') {
        listener = options;
        options = {};
    } else {
        options = options || {};
    }

    if (listener) {
        this.on('session', listener);
    }

    this.tls = options.key && options.cert;
    var transport = net;

    if (this.tls) {
        util.inherits(this, tls.Server);
        transport = tls;
    }

    transport.Server.call(this, options, function(socket) {
        var session = new Session({socket: socket});
        session.server = self;
        self.sessions.push(session);
        socket.on('close', function() {
            self.sessions.splice(self.sessions.indexOf(session), 1);
        });
        self.emit('session', session);
    });
}

util.inherits(Server, net.Server);

Server.prototype.listen = function() {
    var transport = this.tls ? tls : net;
    var args = [this.tls ? 3550 : 2775];
    if (typeof arguments[0] == 'function') {
        args[1] = arguments[0];
    } else if (arguments.length > 0) {
        args = arguments;
    }

    return transport.Server.prototype.listen.apply(this, args);
};

Haven't tested it as finding solutions out of the library is preferable

[chrishornmem]

@JulianGu @farhadi I am also getting this issue. For some reason, it appears when running under a node child process (not using tls but using regular net connection). Applying the code change posted above fixes my issue.

[vfcp]

Here's a minimal script that reproduces the issue. I'll prepare a PR based on @juliangut solution.

const cluster = require('cluster');
const smpp = require('smpp');
const numWorkers = 2;

if (cluster.isMaster) {
    console.log(`Master ${process.pid} is running`);

    // Fork workers.
    for (let i = 0; i < numWorkers; i++) {
        cluster.fork();
    }

    cluster.on('exit', (worker, code, signal) => {
        console.log(`worker ${worker.process.pid} died`);
    });
} else {
    var server = smpp.createServer(function (session) {
        session.on('bind_transceiver', function (pdu) {
            session.send(pdu.response());
        });
    });
    server.listen(2775);

    console.log(`Worker ${process.pid} started`);
}

Output:

Master 13864 is running
_tls_wrap.js:931
  return this._sharedCreds.context.getTicketKeys(keys);
                           ^

TypeError: Cannot read property 'context' of undefined
    at Server.getTicketKeys (_tls_wrap.js:931:28)
    at Server._getServerData (_tls_wrap.js:920:22)
    at EventEmitter.cluster._getServer (internal/cluster/child.js:84:24)
    at listenInCluster (net.js:1338:11)
    at Server.listen (net.js:1412:7)
    at Server.listen (C:\Projects\cluster_mode_fix\node_modules\smpp\lib\smpp.js:176:37)
    at Object.<anonymous> (C:\Projects\cluster_mode_fix\index.js:22:12)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)
worker 9936 died
_tls_wrap.js:931
  return this._sharedCreds.context.getTicketKeys(keys);
                           ^

TypeError: Cannot read property 'context' of undefined
    at Server.getTicketKeys (_tls_wrap.js:931:28)
    at Server._getServerData (_tls_wrap.js:920:22)
    at EventEmitter.cluster._getServer (internal/cluster/child.js:84:24)
    at listenInCluster (net.js:1338:11)
    at Server.listen (net.js:1412:7)
    at Server.listen (C:\Projects\cluster_mode_fix\node_modules\smpp\lib\smpp.js:176:37)
    at Object.<anonymous> (C:\Projects\cluster_mode_fix\index.js:22:12)
    at Module._compile (internal/modules/cjs/loader.js:689:30)
    at Object.Module._extensions..js (internal/modules/cjs/loader.js:700:10)
    at Module.load (internal/modules/cjs/loader.js:599:32)
worker 10832 died

[juliangut]

Given this thread is attracting some attention I can confirm my previous solution out of the library at #79 (comment) has been working on production for 9 months now, if @vfcp prepares a PR and is accepted I'll be happy to update and get rid of my little hack :)

[farhadi]

Just merged the PR.
Thanks for your collaboration @vfcp and @juliangut.