fix: replace p-queue with less restrictive queue #2339
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Adds a `Queue` class to `@libp2p/utils` modelled on p-queue with a few key differences: 1. The queue is externally accessible so we can modify it before jobs run 2. It can be turned into an async generator 3. Jobs remain in the queue while they are executing for better introspection 4. It integrates with libp2p metrics, if desired The dial queue has been replaced with this new queue class, this means we don't need to maintain a separate internal queue for pending dials since the dial queue itself is accessible.
achingbrain
added a commit
that referenced
this pull request
Dec 30, 2023
Remove extra/add missing deps shown by ipfs/aegir#1426 Depends on #2339
wemeetagain
reviewed
Jan 2, 2024
packages/utils/src/queue/index.ts
Outdated
| await this.onEvent('idle') | ||
| } | ||
|
|
||
| private async onEvent (event: keyof QueueEvents<JobReturnType>, filter?: () => boolean): Promise<void> { |
There was a problem hiding this comment.
Does this need to be abortable?
I guess not if clear/abort will allow all pending promises (from public methods, onEmpty, onSizeLessThan, onIdle, toGenerator) to resolve.
There was a problem hiding this comment.
It's not abortable in p-queue, though it makes sense
There was a problem hiding this comment.
They are abortable now
Merged
akim-bow
reviewed
Jan 18, 2024
Comment on lines
+79
to
+85
| "./queue": { | ||
| "types": "./dist/src/queue/index.d.ts", | ||
| "import": "./dist/src/queue/index.js" | ||
| }, | ||
| "./peer-queue": { | ||
| "types": "./dist/src/peer-queue.d.ts", | ||
| "import": "./dist/src/peer-queue.js" |
There was a problem hiding this comment.
@achingbrain This PR introduces BREAKING CHANGE and actually broke many libp2p projects. Why not to bump major version if you break package API? Libp2p packages use caret ranges and automatically will try to update this package when installed.
This was referenced Jan 18, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a
Queueclass to@libp2p/utilsmodelled on p-queue with a few key differences:The dial queue has been replaced with this new queue class, this means we don't need to maintain a separate internal queue for pending dials since the dial queue itself is accessible.
Change checklist