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

Query never gets cancelled regardless of requestTimeout setting. #1485

Closed
AdamJohnSwan opened this issue Oct 27, 2022 · 5 comments
Closed

Query never gets cancelled regardless of requestTimeout setting. #1485

AdamJohnSwan opened this issue Oct 27, 2022 · 5 comments

Comments

@AdamJohnSwan
Copy link
Contributor

Software versions

  • Tedious: 15.1.0
  • SQL Server: Microsoft SQL Server Developer (64-bit) 15.0.2095.3
  • Node.js: 6.17.0

Additional Libraries Used and Versions
My proof of concept only requires ts-node and tedious.

Table schema
No table schema for this.

Connection configuration

{
        server: "localhost",
        authentication: {
            type: "default",
            options: {  
                userName: "testuser",
                password: "password",
            }
        },
        options: {
            trustServerCertificate: true,
            database: "master",
            requestTimeout: 2000
        }
    }

Problem description
Hello, I am running into a problem where a query will run forever regardless of the requestTimeout option. It seems to be the case if the query runs an infinite loop. I thought that when the requestTimeout was reached the query would get cancelled. But that is not the case.

Expected behavior
The query gets cancelled after it reaches my requestTimeout (2 seconds).

Actual behavior
The query will never stop until I shutdown my application.

Error message/stack trace
No error messages, since the query runs forever.

Any other details that can be helpful
I found a way to reproduce it.

Attached is my test file. I changed it to a txt for uploading. Anyway, change it to a .ts file and run with ts-node test.ts.

It makes a connection to a sql database running on localhost with a request timeout of 2 seconds and tries to run this query.

      declare @count int = 0
      while @count < 1
      begin
          continue;
          WAITFOR DELAY '0:10';
          set @count+=1;
      end

Due to the continue statement the while loop never finishes. I thought that the query would get cancelled after two seconds but instead it runs forever. If the continue statement is removed the query will get cancelled after two seconds.

test.txt

@arthurschreiber
Copy link
Collaborator

So, this one is a tough nut. tedious does not cancel requests after receiving the start of the response stream from the server. The reason for that is that we don't know what the client will do with the response and the received rows.

The SQL query you're executing starts immediately sending a response (I think continue causes a DoneInProc token to be send from the server to the client), so the request timeout is immediately cancelled. But the server is stuck in an infinite loop, never actually finishing that request and continuously sending new data to the client.

You can add your own timer and call request.cancel.

@AdamJohnSwan
Copy link
Contributor Author

Alright, I tried implementing it like so:

    const timeout = setTimeout(() => {
        console.log("cancelled")
        conn.cancel()
    }, requestTimeout);

    const request = new Request(query, (err, rowCount) => {
        // The request has finished, the timeout can be cleared.
        clearTimeout(timeout);
        if (err) {
            console.log(err);
        } else {
            console.log(rowCount + ' rows');
        }
    });
    conn.execSql(request);

Seems to work fine.

@AdamJohnSwan
Copy link
Contributor Author

What is your opinion on adding an option for a maximum lifetime of a query to the connection settings? Or adding a TSDoc comment about this somewhere.

@arthurschreiber
Copy link
Collaborator

@AdamJohnSwan Yeah, we definitely need to call out that the requestTimeout on both the connection as well as on individual requests is only for the "request" part of things, as soon as we get a response, the timeout is cleared.

Would you be open to provide a pull request for that?

Long term, I want to replace the timeout options with AbortSignal handling instead. That way, the client can handle request cancellation in a more fine-grained way.

@AdamJohnSwan
Copy link
Contributor Author

Yea I can put a PR together.

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

No branches or pull requests

2 participants