Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
fullsailor opened this issue Aug 16, 2023 · 4 comments · May be fixed by #1530
Open

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

fullsailor opened this issue Aug 16, 2023 · 4 comments · May be fixed by #1530

Comments

@fullsailor
Copy link

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

  • NodeJS: 18
  • node-mssql: 9.1.1
  • SQL Server: 2019

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
Copy link
Collaborator

dhensby commented Aug 16, 2023

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
Copy link
Author

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
Copy link
Collaborator

dhensby commented Aug 16, 2023 •
edited

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 dhensby linked a pull request Aug 16, 2023 that will close this issue
Draft
6 tasks
@dhensby
Copy link
Collaborator

dhensby commented Aug 16, 2023

#1530 is a quick example of what might work.

@dhensby dhensby mentioned this issue Jan 15, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat: add ability to set per-request requestTimeout dhensby/node-mssql
2 participants
@fullsailor @dhensby