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

Remove queued query #2480

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

martinmacko47
Copy link

Resolves #2478

@martinmacko47
Copy link
Author

I will add tests later, after the general approach in this implementation is decided acceptable.

@@ -336,6 +336,14 @@ Protocol.prototype._dequeue = function(sequence) {
return;
}

if (sequence !== this._queue[0]) {
var idx = this._queue.indexOf(sequence);
if (idx > 0) {
Copy link

@maximelkin maximelkin Apr 18, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

>= ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

idx will never be 0 as sequence !== this._queue[0]. The sequence in this._queue[0] is the one that is currently being executed, isn't it?

Or you mean to change it to idx >= 1?

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My bad, missed previous line.

Btw maybe use separate method for removing sequence from queue? Its seems current method executes next sequence, but im not sure.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maximelkin I don't call Protocol._dequeue(sequence) directly. It is called from handler of Sequence.emit('end') emited from Query.end() called in my Query.dequeue(). I wanted to use the native query ending mechanism, so the query will call its _callback (with nothing) after being cancelled.

But yes, if we think it would be better, we could short circuit the native query ending mechanism, and just remove the query from the queue without calling Query.end() and emitting Sequence.emit('end'). But then the query _callback would never be called. I'm not sure if it would be good or not.

if (this._started) {
throw new Error('Query already started.');
}
this.end();
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not entirelry sure calling this.end() here won't mess something. I've tracked the code, it seems to behave as expected: it calls this._callback with no results, emits end which calls Protocol._dequeue(this). But I'm not sure I covered all cases that may happen.

@martinmacko47 martinmacko47 marked this pull request as ready for review April 27, 2021 15:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Remove queued query
2 participants