-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: use blockstore sessions #50
Conversation
In draft because it needs the next Helia release to work and also because the abort-handling tests are skipped pending a refactor - they feature extensive stubbing of internals which have now changed. These need refactoring to land nodejs/node#42 anyway so... |
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.
FYI, i ran tests locally here and I get:
custom dns-resolvers
(node:47763) MaxListenersExceededWarning: Possible EventTarget memory leak detected. 101 abort listeners added to [AbortSignal]. Use events.setMaxListeners() to increase limit
(Use `node --trace-warnings ...` to show where the warning was created)
# lots of these...
(node:47763) MaxListenersExceededWarning: Possible EventTarget memory leak detected. 585 abort listeners added to [AbortSignal]. Use events.setMaxListeners() to increase limit
^C [1m-16.536s]
* - https://gateway.org/ipfs/Qmfoo/bar.txt -> /ipfs/Qmfoo | ||
* - etc | ||
*/ | ||
export function resourceToSessionCacheKey (url: string | CID): string { |
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.
very minor nit that has no actual impact: I would prefer ip[fn]s://${cidOrPeerIdOrDnsLink}
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.
You're right, it makes no difference at all 🤣
I've made the change.
@@ -9,6 +9,7 @@ import { peerIdFromString } from '@libp2p/peer-id' | |||
import { Key } from 'interface-datastore' | |||
import { exporter } from 'ipfs-unixfs-exporter' | |||
import toBrowserReadableStream from 'it-to-browser-readablestream' | |||
import { LRUCache } from 'lru-cache' |
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.
nit: we have src/utils/tlru.ts
we could use instead of pulling in another dep
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.
src/utils/tlru.ts
is based on hashlru
which doesn't have the equivalent of lru-cache
's dispose
option which we need to clean up sessions before they are evicted from the cache.
TBH lru-cache
is 10x more downloaded on npm than hashlru
and it does everything src/utils/tlru.ts
does, maybe we should swap that out for lru-cache
as a follow-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.
good callout, lets do it
Enabling debug messaging:
|
Adds a configurable session cache that creates sessions based on the base URL of the requested resource. E.g. `https://Qmfoo.ipfs.gateway.com/foo.txt` and`https://Qmfoo.ipfs.gateway.com/bar.txt` will be loaded from the same session. Defaults to 100 sessions maximum with a TTL of one minute. These are arbitrary numbers that will require some tweaking.
9b87ff1
to
4b05147
Compare
I think that might actually be a bug in node - nodejs/undici#3157 - there's a workaround here: ipfs/helia#511 |
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.
Code looks good to me. I would love to give this a run locally. Is there anything else we're waiting on here?
Just the Helia release with HTTP Gateway routing - this has shipped now so we're not waiting on anything else 👍 |
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.
reviewed updated changes that came from #73 & merge from main
## @helia/verified-fetch [1.4.0](https://github.com/ipfs/helia-verified-fetch/compare/@helia/verified-fetch-1.3.14...@helia/verified-fetch-1.4.0) (2024-05-09) ### Features * use blockstore sessions ([#50](#50)) ([541dd64](541dd64))
🎉 This PR is included in version 1.4.0 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
## @helia/verified-fetch-interop [1.22.0](https://github.com/ipfs/helia-verified-fetch/compare/@helia/verified-fetch-interop-1.21.1...@helia/verified-fetch-interop-1.22.0) (2024-05-09) ### Features * use blockstore sessions ([#50](#50)) ([541dd64](541dd64)) ### Dependencies * **@helia/verified-fetch:** upgraded to 1.4.0
🎉 This PR is included in version 1.22.0 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 1.0.0 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
Adds a configurable session cache that creates sessions based on the base URL of the requested resource.
E.g.
https://Qmfoo.ipfs.gateway.com/foo.txt
andhttps://Qmfoo.ipfs.gateway.com/bar.txt
will be loaded from the same session.Defaults to 100 sessions maximum with a TTL of one minute. These are arbitrary numbers that will require some tweaking.
Adds
allowInsecure
andallowLocal
options tocreateVerifiedFetch
factory - because we search for gateways in the routing we need to exclude those that have local or http addresses, otherwise we'll get mixed content/connection errors.This makes testing against local gateways difficult so pass
true
for these options to not filter out private/insecure hosts.Depends on:
Change checklist