-
Notifications
You must be signed in to change notification settings - Fork 7
fix: fixes cache leak and unhandled promise rejection #152
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
Conversation
Fixes some bugs in the new request cache:
1. When stopping the client we were supposed to remove the response
cache, instead we were deleting an item from the cache
2. When stopping we were `void`ing the cache deletion - this can
cause unhandeled promise rejections so switches to `await`ing
the promise instead
3. When starting the cache was being created asynchronously but without
waiting for the promise to resolve, so you could stop the client
before the cache was intialized which would mean it is never
cleaned up
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks for catching these.
|
Btw, are you certain that we call start in consumers of the client (Helia and libp2p)?
|
|
Yes, we call |
|
Also sorry for not getting to the review of #151 in time, this PR is me doing it post-merge. |
## [@helia/delegated-routing-v1-http-api-client-v4.2.1](https://github.com/ipfs/helia-delegated-routing-v1-http-api/compare/@helia/delegated-routing-v1-http-api-client-4.2.0...@helia/delegated-routing-v1-http-api-client-4.2.1) (2024-11-19) ### Bug Fixes * fixes cache leak and unhandled promise rejection ([#152](#152)) ([1c54723](1c54723))
|
🎉 This PR is included in version @helia/delegated-routing-v1-http-api-client-v4.2.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Fixes some bugs in the new request cache:
voiding the cache deletion - this can cause unhandled promise rejections so switches toawaiting the promise insteadAlso not a bug but it also makes the cache name configurable.
Change checklist