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

Replace request with fetch (and fix other bugs) #86

Merged
merged 101 commits into from
Feb 19, 2022
Merged

Conversation

qgustavor
Copy link
Owner

@qgustavor qgustavor commented Jan 16, 2022

As issue #64 is stale I plan to at least replace request with fetch to remove the warning that NPM shows when installing the library.

Because the existent build process is old it was causing some issues, so I replaced Rollup with esbuild and updated some dev dependencies to reduce the number of reported vulnerabilities on them to zero.

But, because of that, now the distributed code is not ES5 but ES6 and it require more modern platforms, both Node and browser. Anyway, the important thing is making the library work, adding ES5 support, if someone wishes it, will come later.

Checklist:

  • Fix backpressure in upload function which broke when fixing browser/Deno version;
    • Pausing a PassThrough stream, unlike npm's through, do not stop data from passing thought via a pipe;
    • Solution: implement a PassThrough stream that pauses using Transform;
  • Fix other issues that can arise after fixing the above;
    • Uploading never calls end or complete functions
      • Solution: do not override Stream methods
    • "Promise resolution is still pending but the event loop has already resolved." in Deno
    • "spawn deno ENOENT" in CI
    • "spawn npx ENOENT" in CI
    • "Error: Dynamic require of "abort-controller" is not supported" in Node V14
  • Add test case for don't throw error on empty file #83;
  • Add types;
  • Write new documentation (as the wiki do not allow versioning easily, docusaurus allow);
  • Wait for Ship final version of 17.0.0 standard/eslint-config-standard#208 then fix issues found in files that could not be linted;
  • Wait for Mem leak when keepalive is used node-fetch/node-fetch#1446 then fix eventual issues;
  • Add promise support
  • Release preview version and wait for feedback (remember to add tag);
  • Release 1.0;

Now async/await support is required for building.
As esbuild do not compile to ES5 old browsers are not supported at this moment.
Tests are failing, probably the code that handles uploading got broken in 486cd9d.
@qgustavor
Copy link
Owner Author

qgustavor commented Jan 17, 2022

I updated the branch with some fixes, I tested in Node and I could download some files. Both browser and Deno are not working well, but I will put those aside for the moment.

There are still a important pending issue: downloading only works reliably when using single-connection downloading, trying to use chunked downing results in the server disconnecting (socket hang up).

I'm checking megatools source-code and the logic is pretty the same, but it retries failed chunks. How about creating something to handle chunk errors like megatools? If so, how? Hardcode a exponential backoff like megatools or make something more configurable?

Other possibility is trying to detect what is causing the server to drop the connection. In those changes I added a HTTP(S) agent to keep the connection alive and a default user agent (which should be used anyway), but the issue persists.

Edit: Travis finished, for some reason tests are failing in Node 14 with Error: Cannot find module '@babel/core'. Is it an issue in Ava? It it persists I will remove this version from the CI.

Update Travis Node versions.
Make the API easier to be overridden.
Use a default user-agent and HTTP(S) agent.
Implement again single-connection downloading as is the only reliable way to download at the moment.
Fix the browser version to, at least, load file attributes.
@qgustavor
Copy link
Owner Author

qgustavor commented Jan 17, 2022

I will add a handleRetries arguments to .download() which accepts a function and defaults to the below:

function handleRetries (tries, error, cb) {
  if (tries > 8) {
    cb(error)
  } else {
    setTimeout(cb, 1000 * Math.pow(2, tries))
  }
}

The first argument is the current number of tries, the second is the last error and the third is a callback. Calling the callback with an argument will throw an error in the stream and stop the download. The default logic is similar to the one in megatools but instead of capping the max wait to 4 minutes, it stops the download.

By making it an argument library users can override this logic to match their needs without having to modify the library code. A similar argument can be added to the upload function too.

Edit 1: If someone thinks a function like that needs more arguments or, maybe, even replacing (tries, error, cb) with ({tries, error}, cb) please comment before I publish it.

Edit 2: The default retry handler can be overridden globally by replacing File.defaultHandleRetries with other function.

@qgustavor
Copy link
Owner Author

qgustavor commented Jan 17, 2022

Tests are getting a lot of warnings because of this: node-fetch/node-fetch#1446 I will wait this issue to be fixed first.

Edit 1: I downgraded node-fetch to v2 in order to resume development of this library, I will return to v3 when the above issue gets fixed. Ava tests are passing without warnings now.

In the other hand Ava's tests use the mock server, not real servers, and for some reason they continue hanging up. Retrying chunks sure helped to reduce the number of failed downloads, but there are some cases when some chunks fail more than eight times, so while it was an improvement, it was not the solution. I will need to do more tests first to guess what's going on.

Edit 2: Node's own debugger do not support debugging HTTP(S) requests. Using a debugging proxy changes the behavior (it worked fine when I attached Fiddler). I will try to fix the browser build and debug using a browser.

And remove Node 14 from Travis. I don't know why it's failing and I don't want to fix it.
@qgustavor qgustavor marked this pull request as ready for review January 17, 2022 22:45
@qgustavor
Copy link
Owner Author

qgustavor commented Jan 17, 2022

The code now looks good for me. The only thing missing is node-fetch issue to be fixed. Would be great if someone test this branch.

Edit: I think that's better to try fixing more issues before publishing, like fixing those CI checks that are failing. The issue is that Ava's testing the source code, not compiled esbuild code, using Babel and Babel is failing on Node 14.

Is better to rewrite the tests to use only compiled esbuild code, Some internal functions are not exported, so I will need to make esbuild export a build just for testing that include them. Is better to remove the current browser test code as it's just testing the pure-JavaScript versions of crypto functions and add a test using real browsers later.

Edit 2: I think will be better to replace Travis with Github Actions. From my experience it works better. Also, while a test with real browsers may come later, testing with Deno seems to be a good thing as it's similar to browsers: the only things that change between Node and browser builds is crypto code and HTTP requesting code, and in those two cases the same code used for browsers also work in Deno.

Edit 3: Is not like someone really depends on this project or cares for it - except me - so I will take my own time to fix it.

The idea behind those changes is removing Babel dependencies by making Ava work using ES modules, but them I had to rename everything from .js to .mjs. I could add type: 'module' to package.json but them I think it could cause some issues to users of the library that were using it as a commonjs module.

Well, now the source code is a es module that can be run without any compiling. I wonder if would be better to just export the source code as the module build and just compile for cjs and browsers/deno. Better not: currently using esbuild allows to switch to typescript quite easily.

Browser tests removed: they were doing nothing important, just testing pure-javascript fallbacks, but currently the browser build is failing because the browser version of streams is not working well. Will be replaced with Deno based tests later as Deno is quite close to browsers but do not have the issue of being as slow and resource intensive as a browser.

Moved from Travis to Github Actions.
Just a reminder to myself that implementing MEGA's weird MAC using native functions is not only hard, but makes the code run slower (at least on Node 16).

It's similar to this: https://en.wikipedia.org/wiki/CBC-MAC

But there is a small change on this that makes things harder: the computation result is added to a second ECBC-MAC and the main one resets every chunk.

Also, for some reason, trying to remove this.macs = [] by calculating the final mac in the checkMacBounding function made it slower too. The bad thing is that it increases memory usage, a 16 byte Buffer every 1 MB or so.

If someone wants to fix those things, do the benchmarks and send a pull request.
Fixes #29. Finally. After FOUR years.
Package-lock.json was removed because Travis on 04cd018.

It's returning because GitHub Actions.

I hope it don't start causing issues again. Otherwise I will return to Travis!
I expected to Github Actions to be better.
And it is: now I know why Node 14 was failing.
Or not, it should not, before Babel was compiling the code
so the require "hack" was not an issue.
But is needed for the greater well being of everyone
@qgustavor
Copy link
Owner Author

qgustavor commented Jan 20, 2022

Now, from the issues I want to fix, only Typescript related ones are missing. Nice. And I need to sleep.

Edit: Nevermind, I still need to fix the browser build and make Deno-based tests work.

Edit 2: Also need to add tests for the logout and verification methods. Also move documentation from the wiki to something else where versioning is supported.

Also reduce timeout duration, it was too long.
Tests will be built using esbuild and this file will be replaced with a Deno compatible version when testing.

By compiling using esbuild migrating to Typescript should be easier.
Storage and stream encryption tests can be run on built code, other tests will need to be compiled from source.
Also export stream.mac (otherwise implementing this test would be quite a headache).
@qgustavor qgustavor changed the title Replace request with fetch Replace request with fetch (and fix other bugs) Jan 21, 2022
And remove `return this`
For some reason ts-typescript is not working with the ESM types:
"Parsing error: "parserOptions.project" has been set for @typescript-eslint/parser."

Because of that this file is not being linted at the moment.
@qgustavor
Copy link
Owner Author

I could not resist and I fixed the issue with Deno types. The documentation is updated. I did some simple tests and everything is working well, it's even type checking events, which is great.

- Handle AbortError in the API
- Make stream errors synchronous
- Support single-stream download in browser/Deno
- Handle download cancelling in single-stream downloading
- Remove unneeded process.nextTick since promises are being used
- Simplify promisify function
- Add tests
If you call sml while connections are still open then they will return ESID.
But aborting is not enough, so API ignores responses after it was been closed.
@qgustavor
Copy link
Owner Author

qgustavor commented Feb 13, 2022

Seems chunked download is broken: I just got two corrupted downloads. I think the issue is how the retry mechanism works since storage testing do not test it (mega-mock do not simulate a failed connection).

Edit: well, there is no test downloading the large file uploaded before, so the issue might be other thing. I will change this test to include some deterministic random looking data instead of zeros to detect if chunks are merged in the wrong order.

Edit 2: I'm modifying the streamed upload test to also test downloading... and it's failing.

Edit 3: streamed uploading and chucked downloading are broken.

Edit 4: for some reason source is switching to paused mode and the data stops. Calling read() multiple times make the data events emit again.

Edit 5: Checking documentation I checked the readableFlowing and it's false. Since neither .pause() nor .unpipe() are being called the only reason for that is backpressure. Seems the issue relies in the pauseStream implementation which is not handling backpressure correctly.

Edit 6: Seems it will be better to rewrite the upload function to not depend on neither pauseStream nor pumpify. 1.0 will have to wait.

Edit 7: I changed the upload function to not depend on pauseStream and it's now working in Chrome and Deno, but not in Node. Weird.

Edit 8: When I try to debug it using Node's debugger the bug vanishes. This is a Heisenbug!

@qgustavor
Copy link
Owner Author

qgustavor commented Feb 13, 2022

Another issue: error handling in .upload() got messed because, since a promise is always returned, the error event is never called, instead the promise will be rejected. What to do? Check if the error event have a listener?

Edit: That will be the fix.

Also fix bandwidth limit error and error handling when using streams.
There is still an issue that's causing issues in Deno and browsers, for some reason chunks are being read out of order randomly.
@qgustavor
Copy link
Owner Author

Looks like there is a race-condition in the download function which causes chunks to be read out of order. Weird.

lib/file.mjs Outdated Show resolved Hide resolved
Wrong variable. Also handle backpressure.
Children and parent were missing. MutableFile class is not exported.
@qgustavor
Copy link
Owner Author

qgustavor commented Feb 18, 2022

storage › Should upload and download huge files in parts Promise returned by test never resolved

Looks like streaming upload and/or download are randomly failing. It needs more tests.

Fix potential race condition.
Split upload and download tests.
@qgustavor qgustavor merged commit 9c1d395 into master Feb 19, 2022
@qgustavor qgustavor deleted the replacing-request branch September 3, 2022 01:33
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.

2 participants