OPDATA-6745: More test coverage for Requester#802
Merged
Conversation
Contributor
NPM Publishing labels 🏷️🔵 This PR has the |
2478707 to
76e5ba4
Compare
mxiao-cll
approved these changes
Jun 2, 2026
Contributor
|
❌ failed to create version bump PR |
This file contains hidden or 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
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.
OPDATA-6745
Description
There is a memory leak in
Requester.If requests come in frequently enough that the requester queue is never empty, even if it stays at a small size, the recursive
processNextfunction will chainPromiseobjects forever. Only when the request queue is empty, does the promise stack unravel to release the memory.This happens in
por-address-list,the-network-firm,tiingoand to a lesser extentfinnhub-secondarybecause they have multiple endpoint making multiple requests. This means that even though one endpoint waits for all its requests to finish before sending a new batch, there is always a batch from another endpoint still in the queue to keep the promise chain from unraveling.I plan to significantly refactor the
Requesterto simplify how it queues requests to fix this issue. But before I do, I want to make sure there is extensive test coverage to make sure I don't break anything with the refactoring.This PR adds that test coverage.
Another side effect of how the queue is processed is that some logs that are made by the requester seem to come from a different endpoint than the endpoint that made the request. The logger uses
AsyncLocalStorageto enrich log statements with additional context about the origin of the requests. But becauseRequester.requestwill keep processing queued requests until the queue is empty, requests end up being attributed to the wrong endpoint.While the memory leak is difficult to observe in a unit test, we can observe this
AsyncLocalStorageusage so this will be used as a proxy unit test for whether the memory leak is fixed before confirm it on staging.There are also a few other small bugs observed by unit tests:
None of these bugs are fixed in this PR but they are demonstrated in the tests. They should all be fixed by the follow-up refactoring.
Changes
FixedIntervalRateLimiterwhere it wouldn't rate limit as long asDate.now()equal zero. This was observed in the unit test because it uses fake timer that starts at 0.runAllUntilwhere it could advance time even if it wasn't necessary in order to satisfyisComplete().Requester.