tediousjs / node-mssql

Microsoft SQL Server client for Node.js
https://tediousjs.github.io/node-mssql
MIT License
2.23k stars 465 forks source link

Feature Request: Request.prototype.setTimeout() #1529

Open fullsailor opened 1 year ago

fullsailor commented 1 year ago

I need a way to set different request timeouts without using multiple connection pools. Tedious has a setTimeout function on Request that implements this behavior, but there's not a defined way to get at the driver Request object from a mssql.Request object.

Expected behaviour:

That I could have a pool and request like this, and that the specified timeout would be set on the underlying tedious Request when it is created.

const pool = new ConnectionPool({options: { requestTimeout: 15_000 });
await pool.request().setTimeout(90_000).query("waitfor delay '00:01'"); // synthetic slow query

Actual behaviour:

The only supported workaround is to have different connection pools with different timeouts. This works in some limited cases, but is not very flexible and we can't have two requests in the same transaction with different timeouts.

Software versions

Monkey patch

A bad implementation of this feature would be something like this patch.

declare module 'mssql' {
    interface Request {
        setTimeout(timeout: number | undefined): this;
    }
}

const kTimeout = Symbol('request timeout');
Request.prototype.setTimeout = function (timeout: number | undefined) {
    this[kTimeout] = timeout;
    // for chaining like other mssql Request functions
    return this;
};

// _setCurrentRequest is the only 'hook' that we can patch to get at the underlying
// tedious request before it is passed to the acquired connection
const orig = Request.prototype._setCurrentRequest as (req: any) => any;
Request.prototype._setCurrentRequest = function _setCurrentRequest(req: tedious.Request) {
    if (this[kTimeout] !== undefined) {
        req.setTimeout(this[kTimeout] as number);
    }
    return orig.call(this, req);
};
dhensby commented 1 year ago

Interesting, can you explain a little more about why you need to be able to adjust your timeout on the fly?

Usually I'd expect an application as a whole to share an acceptable timeout, but perhaps there are some cases when it is needed on a connection-by-connection basis.

There are a couple of challenges here:

  1. The connection isn't actually released until the request is made, so this config needs to be held in memory and applied to connections as they are acquired from the pool. But that's not too hard.
  2. We need to make sure this works with the msnodesqlv8 driver too.
fullsailor commented 1 year ago

can you explain a little more about why you need to be able to adjust your timeout on the fly?

Its not so much on the fly, but we have different expected SLAs for specific queries and sprocs. Some kinds of queries we know are slower, so the global timeout has to be raised. In general we want queries that should be fast to fail faster if they hit a 15s timeout.

The connection isn't actually released until the request is made, so this config needs to be held in memory and applied to connections as they are acquired from the pool. But that's not too hard.

I'm specifically trying to reach this API http://tediousjs.github.io/tedious/api-request.html#function_setTimeout from this wrapper package. So, not configuring a connection property but a request one. The life of the tedious.Request is entirely within Request _query/_execute. I also would not expect changing the timeout after calling query/execute to work for any in progress requests.

dhensby commented 1 year ago

I'm specifically trying to reach this API http://tediousjs.github.io/tedious/api-request.html#function_setTimeout from this wrapper package.

I understand, but the point of this package is to provide a generic interface around the underlying connections/requests. So whilst I appreciate you are trying to achieve low-level access, it's not an intended feature to expose that.

So, not configuring a connection property but a request one. The life of the tedious.Request is entirely within Request _query/_execute.

Sure, but that is an academic differentiation, one way or another, the timeout needs to be applied in a one-off manner or within the context of a request.

Looking at it, it should be possible, the question is how to do it nicely. It looks like we could either allow a subset of options to be given to the request:

const pool = await new ConnectionPool(config).connect();
const req = pool.request(someOptionalConfig); // optional config here could contain `requestTimeout`

or we could expose a specific setRequestTimeout() method on requests, but that feels very tightly coupled and doesn't provide any scalability for the future beyond polluting the API surface of Requests.

This change would also need to work for other objects, like PreparedStatement and Transaction, which should follow a similar API.

dhensby commented 1 year ago

1530 is a quick example of what might work.