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

Remove queued query #2478

Open
martinmacko47 opened this issue Apr 14, 2021 · 7 comments · May be fixed by #2480
Open

Remove queued query #2478

martinmacko47 opened this issue Apr 14, 2021 · 7 comments · May be fixed by #2480

Comments

@martinmacko47
Copy link

martinmacko47 commented Apr 14, 2021 •
edited

Would it be possible to add a connection method that would remove a queued (waiting) query from the connection queue? It would take a single parameter the query object as returned from connection.query() and it would remove the query from the queue if the query is waiting. It should respond with one of three states: query removed, query currently executing (can't be removed), query not found (probably already executed).

A rough pseudocode what it would do (specific implementation details will need to be refined):

Connection.prototype.remove = function remove(query) {
  var idx = this._protocol._queue.indexOf(queue)
  if (idx < 0) {
    return false; // Query not found
  }
  if (idx === 0) {
    return false; // Query currently executing
  }
  this._protocol._queue.splice(idx, 1)
  return true;
}

We need this feature for implementing timeouts in Knex.js, see this discussion knex/knex#4416 (comment). We don't want to hack the driver internals and we would prefer to use a supported feature. I'm willing to create a PR with the implementation if the feature would be accepted.

I'm filing a symmetric feature request to mysql2 driver sidorares/node-mysql2#1316. It would be good to implement it in a compatible way in both drivers.
This was referenced Apr 14, 2021
Open
Open
@dougwilson
Copy link
Member

Hi @martinmacko47 thanks for this feature request. I don't see any reason why a feature to remove a queued query cannot be implemented. One tweak I may suggest to our original ideal is the following: what about adding a method like .cancel() or .abort() onto the Query object itself? This would mean you do not have to know what the original connection was the query belongs to (especially helpful with pool.query, in which you never get the original connection object) and this module). A property could also be added to the Query object regarding it's state as well, which may be more useful than just the return value (i.e. you can see if it is executing before attempting a cancel). Thoughts?

@maximelkin
Copy link

maximelkin commented Apr 15, 2021 •
edited

@dougwilson Thank you for your proposal, your solution is much better.
About naming of cancel method - name should show, what method only does job only when query in queue, so users don't have expectations to cancel any query by this method. Maybe .dequeue() or smth?

Also have you any thoughts about pausing queue execution while we cancel current executing query? Potentially cancel is long and we do not want kill next query from execution queue. query.freezeQueue() as example

@martinmacko47 martinmacko47 linked a pull request Apr 18, 2021 that will close this issue
Open
@martinmacko47
Copy link
Author

@dougwilson I agree with the tweak. I created a draft PR #2480 with the implementation. I added Query.started() and Query.ended() methods to determine if the query is pending, already started or already ended. And method Query.dequeue() to remove the query from the connection queue unless it already started.

@martinmacko47
Copy link
Author

@maximelkin

Also have you any thoughts about pausing queue execution while we cancel current executing query? Potentially cancel is long and we do not want kill next query from execution queue.

Won't methods Connection.pause() and Connection.resume() help with this? Or they are for something else?

@maximelkin
Copy link

maximelkin commented Apr 18, 2021 •
edited

@martinmacko47 🤔 they actually for streaming, as per docs.
In default workflow everything is good, but not sure about Connection lost cases (slow connection creation & cancelling? exhausting mysql connection limit on server side?).

@martinmacko47
Copy link
Author

@dougwilson Hi, can you pls check my PR #2480?

@martinmacko47
Copy link
Author

@dougwilson Hello. Is there anything I can do to have the PR checked?

@martinmacko47 martinmacko47 mentioned this issue Jun 21, 2021
Open
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
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Remove queued query martinmacko47/mysql
3 participants
@dougwilson @martinmacko47 @maximelkin