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

fix(ext/kv): send queue wake messages accross different kv instances #20465

Merged
merged 8 commits into from Sep 29, 2023

Conversation

igorzi
Copy link
Contributor

@igorzi igorzi commented Sep 11, 2023

fixes #20454

Current KV queues implementation assumes that enqueue and listenQueue are called on the same instance of Deno.Kv. It's possible that the same Deno process opens multiple KV instances pointing to the same fs path, and in that case listenQueue should still get notified of messages enqueued through a different KV instance.

Copy link
Member

@losfair losfair left a comment

Choose a reason for hiding this comment

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

This still doesn't handle the case where a message is enqueued in one process but the listener is in another process?

I think a better solution would be to periodically (with a very low frequency) wake up the queue listener even if there are no locally-enqueued messages.

ext/kv/sqlite.rs Show resolved Hide resolved
@igorzi
Copy link
Contributor Author

igorzi commented Sep 13, 2023

This still doesn't handle the case where a message is enqueued in one process but the listener is in another process?

I think a better solution would be to periodically (with a very low frequency) wake up the queue listener even if there are no locally-enqueued messages.

Correct, this doesn't take care of cross-process enqueue/dequeue. I propose to handle that in a separate PR, either through a periodic wake-up, or using some cross-process channel (e.g. fs).

@igorzi igorzi requested a review from losfair September 21, 2023 19:47
Copy link
Member

@losfair losfair left a comment

Choose a reason for hiding this comment

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

LGTM

@igorzi igorzi merged commit 61b91e1 into main Sep 29, 2023
13 checks passed
@igorzi igorzi deleted the queues_different_instances branch September 29, 2023 18:40
bartlomieju pushed a commit that referenced this pull request Oct 12, 2023
…20465)

fixes #20454

Current KV queues implementation assumes that `enqueue` and
`listenQueue` are called on the same instance of `Deno.Kv`. It's
possible that the same Deno process opens multiple KV instances pointing
to the same fs path, and in that case `listenQueue` should still get
notified of messages enqueued through a different KV instance.
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 this pull request may close these issues.

KV queues don't work if different KV instances are used for enqueue and queueListen
2 participants