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.

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

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

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

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

fullsailor opened this issue Aug 16, 2023 · 11 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

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
6 tasks
@dhensby
Copy link
Collaborator

dhensby commented Aug 16, 2023

#1530 is a quick example of what might work.

@csod-bganatra
Copy link

csod-bganatra commented Oct 1, 2024

Hey @dhensby what is going on in this issue, I need this feature as well ,
We need to add timeouts for some queries that is not in config,
like high or low,
Can you please let me know about the status of this issue.

@dhensby
Copy link
Collaborator

dhensby commented Oct 1, 2024

The PR is "ready" and I was hoping for some feedback from those that wanted it to let me know if it was the right approach (or even better a bit of testing).

Then I was going to finish it up. I didn't hear anything so I wasn't sure if the demand was still there.

@csod-bganatra
Copy link

Okay got it, @dhensby ,
So once the timeout is reached,
is this timeout different than calling,

request.cancel() 

as currently I am using timeout like this, as it is not natively supported by this package, or it is different?

@dhensby
Copy link
Collaborator

dhensby commented Oct 3, 2024

Good question, the change I authored just uses the underlying driver's default behaviour for per-request timeouts. I suspect you're right that it essentially is just calling cancel on the request after a set time.

@csod-bganatra
Copy link

Yes @dhensby can you please confirm this,
so I can go ahead,
as once query is fired I don't think we can cancel the query ?
It is just that it will close the connection, right?

@dhensby
Copy link
Collaborator

dhensby commented Oct 3, 2024

We don't close connections when we cancel queries because the connection is put back into the pool. The cancel call just fires whatever the underlying library does, you'll have to look into that if you want a precise idea of exactly what is going on under-the-hood.

@csod-bganatra
Copy link

Okay got it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants