Move pooling out of node driver?

[benmerckx]

I implemented transactions last night, but couldn't get the tests to pass for node. I can see why after looking at the mysql package sources: https://github.com/mysqljs/mysql/blob/09b308f3fbb8191c2927f259146816b53aae5fa4/lib/Pool.js#L202 Running a query on a pool essentially fetches a connection from the pool and then releases it to the pool again. Which means your queries are potentially run on a different connection each time. This renders statements like START TRANSACTION useless as these operate in a single connection. Furthermore, looking at this comment: https://github.com/mysqljs/mysql/issues/1283#issuecomment-160042731 it seems advisable pick a connection of the pool per request (in a web context). As tink_sql has no knowledge of which context it's running queries in I think it's probably best to make the use of a connection pool explicit. For example by adding:

interface ConnectionPool<Db> {
  function connect(use: Db -> Promise<Noise>): Void;
}
var pool = tink.sql.drivers.node.MySql.createPool(config);
pool.connect(db -> doStuff);

Where you'd get a Db instance whose connection is released when the returned promise is resolved. Any thoughts?

[back2dos]

Well, this is quite a bit beyond me, but here's what I think:

If you take a connection from the pool for every request and release it only after you're done, you can only handle as many requests as you have connections.

I guess what we really want is to isolate transactions. So on a connection you would have:

interface Connection {
  function transaction<T>(f:Connection->Promise<T>):Promise<T>
}

On a non-pooled connection this should probably "lock" the connection, queuing up any subsequent queries and transitions not issued on the argument of f until the promise returned by f resolves.

As for pooled connections, it depends on how the pooling works. In a pure Haxe pool, one would "simply" pick an available connection and forward to it's transaction. But I guess for the nodejs+mysql case we could also use try to build it around their own pool (not that I particularly enjoy relying on JS code, but in this case it's battle tested).

The missing bit would be to generate the following in every Database subclass:

public function transaction<T>(f:TheSubClass->Promise<T>):Promise<T>
  //create a new TheSubClass with the connection obtained from the transaction

This will require an optional connection argument in the constructor, so the argument for f can be initialized with the connection coming from the pool.

Do you think this would work for you? It should definitely isolate the transaction. But you'll have to be careful to operate on the argument of f rather than object that you called transaction on.

[benmerckx]

Thanks, that gives me a lot to work with.

This will require an optional connection argument in the constructor, so the argument for f can be initialized with the connection coming from the pool.

True, but typing it is a bit of a problem though. I tried building the type in DatabaseBuilder, but then the field is only added after the build step. The compiler complains about using the cnx variable in regular methods. I've resorted to Connection<Any>. Maybe a function getConnection(): Connection<Any> and overriding that in the DatabaseBuilder to be specific would work. But it doesn't seem worth it?

[back2dos]

Hmm, I was thinking about just modifying the generated constructor:

    var ctor = c.getConstructor((macro function (name, driver:tink.sql.Driver, ?cnx) {
      if (cnx == null) cnx = driver.open(name, this);
      $b{init};
      super(name, driver, $a{tables});
    }).getFunction().sure());

Type inference should do the rest, unless I'm missing something?

[benmerckx]

You'd need to access it in the transaction method (to call the underlying transaction method on the connection), so a reference is needed?

[back2dos]

In that case I'd store it in an instance variable on the subclass.

[kevinresol]

Is there any update about transactions?

[benmerckx]

Not on my end. It ended here for me, but then I ran into the pooling issues: https://github.com/benmerckx/tink_sql/commit/dae29a1e8b212a11f24176de3b5bc7e741e9b3ad

[kevinresol]

But you'll have to be careful to operate on the argument of f rather than object that you called transaction on.

This should be fine as long as the commit()/rollback() calls exist only in the TheSubClass, that is enough hint already.

[kevinresol]

I am not sure if it is related, but some time ago I added true streaming support to the node driver, which involves manually obtaining a connection from the pool and releasing it when the stream is fully consumed: https://github.com/haxetink/tink_sql/blob/097c652f7be854385e933730c17f7f376595156f/src/tink/sql/drivers/node/MySql.hx#L138

[kevinresol]

Perhaps it is actually easier to support transaction by moving pooling out of the node driver (and perhaps do the pooling in tink-level)?

cc @andyli