connection pooler

[e11137]

Hi,
thanks for your excellent work.
I have integrated a connection poller based on generic-pool " https://github.com/coopernurse/node-pool".

may be It can be merged ?

thanks

Best regards

[pekim]

Thank you, this looks interesting.
I confess that I've only had a brief look at this code and at node-pool, and I might be misunderstanding the code, but I have some concerns.

It slightly worries me that Connection's api is duplicated in ConnectionPooler. This would be a maintenance overhead; any changes to Connection's api would have to be reflected in ConnectionPooler.

However my bigger concern is that the implementation simply may not work. It looks to me like a connection is acquired from the pool for each request. If that is the case, then some requests will not work. For example, you cannot begin and end a transaction on different connections. Have you tested this sort of thing?

That brings me on to testing. I'd like to see integration tests, exercising all of ConnectionPooler's function.

Sorry if this all seems a bit negative. I don't want to discourage contributions.

[e11137]

Hi,
could you check my modifications on your connection.coffee object.
I need this event to release connections in my pooler. Without it, I have a dead lock.

https://github.com/e11137/TediousPooler.git

thanks

[pekim]

It's the version of connection.coffee at https://github.com/e11137/tedious/blob/master/src/connection.coffee, with the emmision of reusable events that you'd like me to take? It looks good, however there's one small problem.

    SENT_CLIENT_REQUEST:
      name: 'SentClientRequest'
      exit:->
        @reusable = true
      events:
        socketError: (error) ->
          @transitionTo(@STATE.FINAL)
        data: (data) ->
          @sendDataToTokenStreamParser(data)
        message: ->
          sqlRequest = @request
          @request = undefined
          sqlRequest.callback(sqlRequest.error, sqlRequest.rowCount)

          @transitionTo(@STATE.LOGGED_IN)

Moving the state transition to the end of the message event causes some integration test failures, including a stack overflow. Is there a particular reason for this change?

Moving it back to the start of the event, all tests pass again.

    SENT_CLIENT_REQUEST:
      name: 'SentClientRequest'
      exit:->
        @reusable = true
      events:
        socketError: (error) ->
          @transitionTo(@STATE.FINAL)
        data: (data) ->
          @sendDataToTokenStreamParser(data)
        message: ->
          @transitionTo(@STATE.LOGGED_IN)
          sqlRequest = @request
          @request = undefined
          sqlRequest.callback(sqlRequest.error, sqlRequest.rowCount)

[e11137]

I have an error when I move it back.

Error: Received 'columnMetadata' when no sqlRequest is in progress
    at Parser.Connection.createTokenStreamParser.tokenStreamParser.on._this.procReturnStatusValue (/home/rcanedo/github/public/tedious/lib/connection.js:326:15)
    at Parser.EventEmitter.emit (events.js:88:17)
    at Parser.nextToken (/home/rcanedo/github/public/tedious/lib/token/token-stream-parser.js:86:18)
    at Parser.addBuffer (/home/rcanedo/github/public/tedious/lib/token/token-stream-parser.js:65:17)
    at Connection.sendDataToTokenStreamParser (/home/rcanedo/github/public/tedious/lib/connection.js:538:35)
    at Connection.STATE.SENT_CLIENT_REQUEST.events.data (/home/rcanedo/github/public/tedious/lib/connection.js:197:23)
    at Connection.dispatchEvent (/home/rcanedo/github/public/tedious/lib/connection.js:444:59)
    at MessageIO.Connection.connectOnPort (/home/rcanedo/github/public/tedious/lib/connection.js:398:20)
    at MessageIO.EventEmitter.emit (events.js:88:17)
    at MessageIO.eventData (/home/rcanedo/github/public/tedious/lib/message-io.js:57:12)

you can reproduce this error with this code.

var ConnectionPooler = require('../lib/tedious').ConnectionPooler;
var Request = require('../lib/tedious').Request;

var config = {
  server: '192.168.1.212',
  userName: 'test',
  password: 'test'
}

var pooler = new ConnectionPooler(config);

for(i=0;i<15;i++)
pooler.execute(function(connection){
   var request = new Request("select 42, 'hello world'", function(err, rowCount) {
      if (err) {
         console.log(err);
      } else {
         console.log("Pooler "+rowCount + ' rows');
      }
   });

   request.on('done', function(rowCount, more) {
      console.log(rowCount + ' rows returned');
   });
   connection.execSql(request)
});

[pekim]

The state machine has to be in the correct state before calling the request's completion callback. The API allows another request to be made on the connection as soon as the callback is called. If the state machine is not put in to the correct state before the request completion callback is called, then initiating another request on the connection during the callback would fail (as there's code to detect when the connection is not in a suitable state).

I could swap the execution order, to call the callback first. I'd then have to change the documentation to reflect this. This would force any code that needs to initiate another request on the completion of a request, to defer the new request to the next tick. I don't want to do this unless we have to.

I'd prefer to keep the current execution order, and work out why this breaks the pooler code. Then fix either tedious or the pooler (or generic-pool) once the problem is better understood.

At the moment I'm baffled about what's causing the problem, as the pooler code looks perfectly reasonable. I think I'm going to have to scatter some console.log statements around in connection.coffee, connection-Pooler.coffee and in the generic-pool code to understand the execution order better.

[pekim]

TediousPooler.prototype.execute = function(callback) {
   var self = this;
   this.pool.acquire(function(err, connection) {
      if(err) {
         self.looger.log("TediousPooler :"+err,'error');
      }
      else {
         callback(connection);
         self.pool.release(connection);
      }
   });
}

The connection is released back in to the pool (and is therefore available for re-use) as soon as the callback has been called. If I'm reading the code correctly there is no guarantee, in fact it's very unlikely, that the request on the connection has completed at that point. So putting the connection back in to the pool, and then immediately calling pooler.execute may well use the same connection.

I added some log statements in TediousPooler, and this does seem to be what is happening. It is reusing the same connection for every request, even though it hasn't completed the first request.

[e11137]

Hi,
this code is from TediousPoller https://github.com/e11137/TediousPooler.
I have made some tests to use it without reusable event but it doesn't work for the moment.

My first exemple use var ConnectionPooler declared in https://github.com/e11137/tedious/blob/master/src/connection-Pooler.coffee (I need to change the emition of reusable event to

create: (callback) ->
        connection = new Connection(config)
        connection.on('connect', (err) ->
          console.log('connected')
          if !err
            callback(null, connection)
            connection.on('reusable', () ->
              _this.pool.release(connection)
            )
        )

I need to change the emission of reusable event to allow multiple requests in the same callback and only emit one event at the end.

[pekim]

I've been thinking about your approach to the pooling, and I'm not sure that it's going to be easy to make it work. As you say, the tedious API will need to change to allow batches of requests to execute, and only emit one reusable event when all of the requests have completed.

I've been considering an alternative (slightly more conventional) approach, which does not require any changes to tedious. The pooling implementation manages the allocation of Connections. The application requests a connection from the pool, and uses it with the existing API, executing as many requests against the Connection as it wants. When it's finished its work, it calls the close function on the Connection, which returns it to the pool.

I've knocked together an implementation to illustrate this approach. It's at https://github.com/pekim/tedious-connection-pool.

[pekim]

Connection pooling is now a separate project, tedious-connection-pool.