Connection timeout

[rmruano]

options.connectTimeout support for clients + tests + connection error handling doc

[coveralls]

Coverage increased (+0.3%) to 78.99% when pulling 3ce9e7c on rmruano:feature/timeout into 4ea9684 on farhadi:master.

[coveralls]

Coverage increased (+0.3%) to 78.99% when pulling 3ce9e7c on rmruano:feature/timeout into 4ea9684 on farhadi:master.

[0LEG0]

You should provide default value of options.connectTimeout for Session constructor too.
Because of this timer will executes ASAP and any connections created via new Session will get "ETIMEOUT" error.

[rmruano]

Due how this library was designed there are 2 workflows sharing the Session object: client & server.

Connection timeout is only implemented for client connections: the session is created synchronously on line 333. In client mode, the session is what makes the connection. In server mode, the connection is already made when the Session object is instantiated.

It's confusing indeed and a major refactor would be a good idea.

[0LEG0]

I had the carelessness to upgrade to the new version of the library and got following bug:
All of new sessions drops just after its creation (for example: var client = new smpp.Session(options) ) with "ETIMEOUT" error. I've spend the time to found the bug but found your new feature :-)

[rmruano]

If no connectTimeout option is provided, there's a default timeout of 30 seconds, nobody should have any ETIMEOUT issues other than when a real connection timeout happens.

The following client tests are passing:
✓ should successfully bind a transceiver with a hardcoded user/password
✓ should fail to bind a transceiver with a wrong hardcoded user/password
✓ should successfully emit every expected debug log entry
✓ should fail to connect with an invalid port and trigger a ECONNREFUSED error
✓ should fail to connect with an invalid host and trigger a EAI_AGAIN, ENOTFOUND or ESRCH error
✓ should fail to connect with an invalid host and trigger a ETIMEOUT error

I also tested a real application to make sure it works as expected.

Anyway, if you have some unexpected behaviour please file an issue and I'll take a look yo your specific case and maybe write a couple more tests if required.

[0LEG0]

Ok, you can reproduce this bug trying something like this:

const smpp = require("smpp");
// Let's create server ...
const server = smpp.createServer().listern({
    host: "127.0.0.1",
    port: 2775,
    family: 4
}, () => {
// ... and connect to it
    const client = new smpp.Session({host: "127.0.0.1", port: 2775});
})

Ofcourse, you can avoid this bug if the client connection creates with smpp.connect().

[rmruano]

Got it.

You still don't have to instantiate sessions by yourself. smpp.connect(url, listener) allows you to provide custom options as a first parameter, everything you set there will end in the transport.connect(options)

The problem is that the parameter is called url, not options, I'll change it and make a PR to avoid any misunderstanding.

I'd advocate for not exporting the Session function and forcing the factory method connect but that would be a BC break... So, it is what it is for now.

[0LEG0]

I tried playing with this library about a year ago and found that not all options from smpp.connect (options) are passed to the socket. Maybe this has changed since then.
I see no great reason to restrict socket creation to just a factory function.

[rmruano]

Don't know how it has evolved, I'm new around here. Anyway, you can check the connect method, everything is being carried into the session and into the socket connection.

About how to instantiate it, what I don't like is having multiple methods of doing the same thing, it's a lot more error prone, like this particular case. Just FYI, there are no tests instantiating the Session directly, you should stick to the connect method, which is the only one being tested for client connections.

To fix your issue I'd add a condition to only apply the timeout if the value of connectTimeout is present and greater than 0. I should have done it.

[0LEG0]

I suggest just moving your default values to the constructor and everything will be the same.
0LEG0@79a0980
Thanks.

[rmruano]

Yes, It could be done and it will work, but then the connect method looses all it's purpose, which is validating and preparing the options object, that's why there aren't any option checks within the Session itself, scattering options checks all along the code is not a good practice and it's very confusing. Anyway, I don't really like how the session works and how it tries to share functionality between the server/client modes, IMHO a major refactor would be the best option.

For now, I think a simple options.connectionTimeout>0 check is more than enough to fix it and it would also work if someone provides a negative number.