request callback not called on socket error

[ashelley]

Hello,

When a socket error occurs the request callback is never called. I've created a proof of concept patch to show how to work around the issue. The goal of this patch is to ensure that when a request exists on the connection the connection should ensure that the request callback gets called.

ashelley@a9ef6e2

@@ -1055,7 +1055,10 @@ Connection.prototype.STATE = {
   SENT_CLIENT_REQUEST: {
     name: 'SentClientRequest',
     events: {
-      socketError: function() {
+      socketError: function(err) {
+        const sqlRequest = this.request;
+        this.request = void 0;
+        sqlRequest.callback(err);
         return this.transitionTo(this.STATE.FINAL);
       },
       data: function(data) {

Here is my test code to show that the callback is never called:

var tedious = require('tedious');
var Request = tedious.Request;
var Connection = tedious.Connection;

var poolConfig = {
    min: 2,
    max: 4,
    log: function(text) {
        console.log('log', text);
    }
};

var sqlConfig = {
    userName: process.env.SQL_USER,
    password: process.env.SQL_PASS,
    server: process.env.SQL_HOST,
    options: {
        port: process.env.SQL_PORT,
        database: process.env.SQL_DATABASE,
        requestTimeout: 0
    }
};

var counter = 0;

function getConnection(done) {
    counter++;
    var connection = new Connection(sqlConfig);
    connection.on('connect', function(err) {
        done(err, connection);
    });
    connection.on('error', function(err) {
        console.log('connection error', err);
    });
}

function execSql(connection, sql, done) {
    var request = new Request(sql, done);
    connection.execSql(request);
}

function start() {
    getConnection(function(err, connection) {
        if (err) {
            console.log('error acquiring connection', err);
            return next();
        }
        console.log('querying');
        execSql(connection, "WAITFOR DELAY '00:01';\nSELECT NEWID();", function(err, rowCount) {
            if (err) {
                console.log('error executing query', err);
                return next();
            }
            console.log('success', rowCount);
            next();
        });
    });
}

function next() {
    console.log('next');
    setTimeout(function() {
        start()
    }, 5000);
}

start();

While running the above program after I see it start querying I block the port to simulate a socket error:

# iptables -A OUTPUT -p tcp --dport $SQL_PORT -j DROP

[lee-houghton]

Interesting, this could well be the source of the resource leak woes I've been having in production. I'll try it out and report back sometime tomorrow hopefully.

[ashelley]

fyi, i think the patch should be:

       socketError: function(err) {
         const sqlRequest = this.request;
         this.request = void 0;
         sqlRequest.callback(err);
         return this.transitionTo(this.STATE.FINAL);
       },

made an error with what version i pushed, it wasn't passing along the socket error

[bretcope]

Would you mind turning your patch into an actual pull request? Also, adding a test to the test suite would be helpful.

[ashelley]

hello @bretcope I have created a pull request and added a simple test here:

#325