feat: use blockstore sessions #50
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
|
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... |
4 tasks
SgtPooki
requested changes
Apr 17, 2024
There was a problem hiding this comment.
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.
very minor nit that has no actual impact: I would prefer ip[fn]s://${cidOrPeerIdOrDnsLink}
There was a problem hiding this comment.
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.
nit: we have src/utils/tlru.ts we could use instead of pulling in another dep
There was a problem hiding this comment.
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?
Enabling debug messaging: |
This was referenced Apr 17, 2024
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.
achingbrain
force-pushed
the
feat/use-blockstore-sessions
branch
from
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 |
5 tasks
SgtPooki
approved these changes
Apr 30, 2024
Just the Helia release with HTTP Gateway routing - this has shipped now so we're not waiting on anything else 👍 |
github-actions bot
pushed a commit
that referenced
this pull request
May 9, 2024
## @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 📦🚀 |
github-actions bot
pushed a commit
that referenced
this pull request
May 9, 2024
## @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 was referenced May 9, 2024
Merged
|
🎉 This PR is included in version 1.0.0 🎉 The release is available on npm package (@latest dist-tag) Your semantic-release bot 📦🚀 |
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 configurable session cache that creates sessions based on the base URL of the requested resource.
E.g.
https://Qmfoo.ipfs.gateway.com/foo.txtandhttps://Qmfoo.ipfs.gateway.com/bar.txtwill 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
allowInsecureandallowLocaloptions tocreateVerifiedFetchfactory - 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
truefor these options to not filter out private/insecure hosts.Depends on:
Change checklist