Skip to content
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

fix: client close blockstore on store directory and store blob #1198

Conversation

vasco-santos
Copy link
Contributor

@vasco-santos vasco-santos commented Jan 31, 2022

Given we use a CarReader now, we can only close the blockstore once pack is finished.

Also updated @web-std/fetch to avoid Error: Cannot find module '/home/runner/work/nft.storage/nft.storage/node_modules/@web-std/fetch/dist/index.cjs'

Closes #1186


try {
const { cid, car } = await NFTStorage.encodeBlob(blob, blockstore)
await NFTStorage.storeCar(service, car)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can as an alternative do this within storeCar, but we would need to provide blockstore here, which feels odd as we don't even use ipfs-car related things in the function. Also, wrapping everything to guarantee error handling looked the best option

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my opinion, this can be redesigned as if a user want to store a big chunk, let's say 10GB, they need to have it available in tmpfs at once. I would recommend to have only the last pending blocks in the tmpfs and delete them after uploading.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are working on a new solution to pack files that will stream directly instead of using a temporary blockstore to keep the blocks. So, this will be an enhancement that we will ship soon to improve this behaviour #980

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Jan 31, 2022

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: cbc9eee
Status: ✅  Deploy successful!
Preview URL: https://55147c2c.nft-storage-1at.pages.dev

View logs

@vasco-santos vasco-santos force-pushed the fix/client-close-blockstore-on-store-directory-and-store-blob branch 3 times, most recently from 1616099 to 92eda73 Compare January 31, 2022 15:04
@vasco-santos vasco-santos force-pushed the fix/client-close-blockstore-on-store-directory-and-store-blob branch from 92eda73 to cbc9eee Compare January 31, 2022 15:09
@codecov-commenter
Copy link

Codecov Report

Merging #1198 (14b1071) into main (f26c739) will not change coverage.
The diff coverage is n/a.

❗ Current head 14b1071 differs from pull request most recent head cbc9eee. Consider uploading reports for the commit cbc9eee to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##              main     #1198   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            5         5           
  Lines         1136      1169   +33     
=========================================
+ Hits          1136      1169   +33     
Impacted Files Coverage Δ
src/lib.js 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fd49f4b...cbc9eee. Read the comment docs.

Copy link
Contributor

@hugomrdias hugomrdias left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

tmpfs is not cleared after adding files with storeDirectory
4 participants