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: upload directory with subdirectories [WIP] #624

Merged
merged 2 commits into from
Mar 24, 2018
Merged

Conversation

hacdias
Copy link
Member

@hacdias hacdias commented Mar 21, 2018

Fixes #613.

@hacdias hacdias changed the title fix: upload issue fix: upload directory with subdirectories Mar 21, 2018
@hacdias hacdias changed the title fix: upload directory with subdirectories fix: upload directory with subdirectories [WIP] Mar 21, 2018
@lidel
Copy link
Member

lidel commented Mar 21, 2018

Odd stuff. Just tested it, adding 2 layers of files worked.. fine. Once.

Other one.. failed:

desktop Error: file does not exist
  desktop     at parseError (/home/lidel/project/ipfs-desktop/node_modules/ipfs-api/src/utils/send-request.js:16:17)
  desktop     at ClientRequest.<anonymous> (/home/lidel/project/ipfs-desktop/node_modules/ipfs-api/src/utils/send-request.js:39:14)
  desktop     at Object.onceWrapper (events.js:316:30)
  desktop     at emitOne (events.js:115:13)
  desktop     at ClientRequest.emit (events.js:210:7)
  desktop     at HTTPParser.parserOnIncomingClient (_http_client.js:565:21)
  desktop     at HTTPParser.parserOnHeadersComplete (_http_common.js:116:23)
  desktop     at Socket.socketOnData (_http_client.js:454:20)
  desktop     at emitOne (events.js:115:13)
  desktop     at Socket.emit (events.js:210:7) +76ms

Also, I think I've found kinda unrelated bug when adding bigger directory trees:

Tested against revision 3ded93e
To replicate, just try to add ipfs-desktop sources with .git via "add directory" icon.
Do you get the same error as below?

screenshot_2

May be that my system has low limits, but worth addressing at some point.
Debug log from adding ipfs-desktop on my box:

$ DEBUG='desktop' yarn start
$ electron-forge start
✔ Checking your system
✔ Locating Application
✔ Preparing native dependencies: 3 / 3
✔ Launching Application
  desktop Application is ready +0ms
  desktop Starting daemon +4ms
  desktop Daemon started +3s
  desktop Configuring Stats Poller +0ms
  desktop TypeError: stream.pipe is not a function
  desktop     at Promise (/home/lidel/project/ipfs-desktop/src/controls/main/pinned-files.js:30:12)
  desktop     at Promise (<anonymous>)
  desktop     at collect (/home/lidel/project/ipfs-desktop/src/controls/main/pinned-files.js:29:10)
  desktop     at ipfs.pin.ls.then.then.then.res (/home/lidel/project/ipfs-desktop/src/controls/main/pinned-files.js:76:56)
  desktop     at <anonymous>
  desktop     at process._tickCallback (internal/process/next_tick.js:188:7) +105ms
  desktop Uploading files { files: [ '/home/lidel/project/ipfs-desktop' ] } +12s
  desktop Uncaught Exception: Error: connect EMFILE 127.0.0.1:37895 - Local (undefined:undefined)
  desktop     at Object.exports._errnoException (util.js:1024:11)
  desktop     at exports._exceptionWithHostPort (util.js:1047:20)
  desktop     at internalConnect (net.js:934:16)
  desktop     at net.js:1033:9
  desktop     at _combinedTickCallback (internal/process/next_tick.js:131:7)
  desktop     at process._tickCallback (internal/process/next_tick.js:180:9) +16s

@hacdias
Copy link
Member Author

hacdias commented Mar 21, 2018

@lidel as far as I've tried, there is some kind of limit and I didn't understand it yet because I remember asking someone and there wasn't supposed to be one. I'll try to replicate.

@hacdias
Copy link
Member Author

hacdias commented Mar 21, 2018

@lidel trying on linux what happened was:

  1. Tried to upload
  2. Nothing apparently happened
  3. Tried to upload again
  4. Error file already exists
  5. Refreshed Desktop and found out that it was addedd on the first time.

What I think is that there is some problem updating the pane. I'll solve it 😄

Although the EMFILE error I have never seen it before. I have no idea of what it can be. For what I read, EMFILE means too many open files. Maybe I should not try to add everything at once but like 10 each time to avoid that error.

@hacdias hacdias force-pushed the upload-issue branch 2 times, most recently from 60008b7 to 6ec7fb5 Compare March 22, 2018 09:04
@codecov-io
Copy link

codecov-io commented Mar 22, 2018

Codecov Report

Merging #624 into master will increase coverage by 0.08%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #624      +/-   ##
==========================================
+ Coverage   14.84%   14.92%   +0.08%     
==========================================
  Files          57       57              
  Lines         896      891       -5     
==========================================
  Hits          133      133              
+ Misses        763      758       -5
Impacted Files Coverage Δ
src/panes/Files.js 0% <0%> (ø) ⬆️
src/config.js 0% <0%> (ø) ⬆️
src/controls/utils/upload-files.js 0% <0%> (ø) ⬆️

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 de98150...b57e06d. Read the comment docs.

@hacdias
Copy link
Member Author

hacdias commented Mar 22, 2018

@lidel @olizilla I think b43edb0 actually solves this problem. I created a async function which uploads everything synchronously, file by file, directory by directory.

The file does not exist error was created because since I was using Promise.all, it could eventually try to upload files to directories that didn't exist yet.

I ask you to try this out please and let me know if it works 😄

Edit: this function I just created can be worked to be a bit faster allowing the program to add multiple files at once. For now it is safe and works, but can be faster. I'll work on that on the weekend. Tomorrow I'll probably not be responsive but I'll try to take a look at the notifications and the IRC once in a while.

@lidel
Copy link
Member

lidel commented Mar 23, 2018

I got you some quick feedback before going to 🛌 💤
Performed my usual test (adding ipfs-desktop project tree ~600MB) and good news is that the error changed, and problems moved to bigger directory trees.

I got this Error: blockservice is closed perk after about 3 minutes: log1.txt.
After I start it again I see the ipfs-desktop is only 64MB in size, so it seems app stopped adding stuff at that offset. I was not able to replicate it again tho.

I tried again, and this time got ENOENT: no such file or directory around 240MB: log2.txt. This may be a false-positive as I actually do not have this file on my disk (possibly that electron changed dir tree since adding started?).

For the final test I've added my ipfs-companion project dir instead (~845MB as it includes different versions of Firefox), and after >1h it ended with >600MB added and ECONNREFUSED error:
log3.txt.

Hope this helps :)

@hacdias
Copy link
Member Author

hacdias commented Mar 23, 2018

@lidel,

  • Error: blockservice is closed I never saw that but after some investigation, I found out that @Stebalien is thinking of ignoring this error (Consider ignoring errors from the exchange in the blockservice kubo#4623) and I'm not sure what do to to prevent it.
  • ENOENT: no such file or directory Yeah, something that initially was on the directory you wanted to upload was removed. Namely /home/lidel/project/ipfs-desktop/node_modules/electron-winstaller/node_modules/asar/test/expected/packthis-unpack-dir-glob.asar.unpacked/x1/file1.txt. Do you think we should bypass this error without failing?
  • ECONNREFUSED IPFS refused to connect? Why? Overloaded?

@hacdias
Copy link
Member Author

hacdias commented Mar 23, 2018

I think this was it. @olizilla gave me the idea of using ipfs.add and then ipfs.files.cp to the MFS because ipfs.add supports adding directories directly.

Although, if I add a dir using ipfs.add it returns an array with the hash of every added file. For what I saw, the root directory is always the last element, but I'd like to know if you can confirm that.

@olizilla
Copy link
Member

This works for my small test case that is 2 dirs deep with 3 files.

Trying @lidel's test case of adding the ipfs-companion src tree (with node_modules) causes

  desktop Uploading files { files: [ '/Users/oli/Desktop/foo' ] } +28s
  desktop Uploading files { files: [ '/Users/oli/Code/ipfs-shipyard/ipfs-companion' ] } +1m
  desktop Error: connect EMFILE 127.0.0.1:57379 - Local (undefined:undefined)
  desktop     at Object.exports._errnoException (util.js:1024:11)
  desktop     at exports._exceptionWithHostPort (util.js:1047:20)
  desktop     at internalConnect (net.js:934:16)
  desktop     at net.js:1033:9
  desktop     at _combinedTickCallback (internal/process/next_tick.js:131:7)
  desktop     at process._tickCallback (internal/process/next_tick.js:180:9) +7s
  desktop Uncaught Exception: Error: EMFILE: too many open files, open '/Users/oli/Code/ipfs-shipyard/ipfs-companion/node_modules/choo/example/node_modules/browserify/test/multi_entry_cross_require/c.js' +1s

where

  desktop Uploading files { files: [ '/Users/oli/Desktop/foo' ] } +28s

is the simple test case working, which is new and good news ✨ but then there is a new error with the /Users/oli/Code/ipfs-shipyard/ipfs-companion which is a has many files.

It then continues to try and add all the files, but each one throws a new too many open files error, so now I've got hundreds of error alert boxes to close 💥

@lidel
Copy link
Member

lidel commented Mar 23, 2018

Now I get the same error as @olizilla (EMFILE: too many open files for ipfs-companion tree).

But it is a good news, means we are just missing some way to limit the number of file handlers used in parallel during the add.

Seems that Beaker/Dat team had similar problem and graceful-fs helped with intensive open+close flows: dat-ecosystem/dat#570

@hacdias
Copy link
Member Author

hacdias commented Mar 24, 2018

@lidel yeah, I'll merge this and open an issue on js-ipfs-api after getting a reproducible example.

Well, I'm trying to reproduce it with a simple script and I can't. I noticed that go-ipfs, when it starts, it pushed the file descriptor limit to 2048. Maybe when we start IPFS inside Electron it doesn't happen. I'll add a line to the code to push up the File Descriptor and ask you to try it out please.

@lidel
Copy link
Member

lidel commented Mar 24, 2018

Hm.. this does not seem to make any difference, same as #624 (comment)

@hacdias
Copy link
Member Author

hacdias commented Mar 24, 2018

Hmm. Can you try adding it manually via a script using jsipfsapi? I can't reproduce the EMFILE error in any Linux virtual machine I have.

@hacdias hacdias merged commit d7ab24d into master Mar 24, 2018
@hacdias hacdias deleted the upload-issue branch March 24, 2018 18:25
@hacdias
Copy link
Member Author

hacdias commented Mar 24, 2018

I merged this and I'll open a new issue for the EMFILE errors.

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.

4 participants