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

Uploading crashes the application #48

Closed
ghost opened this issue Jan 10, 2019 · 10 comments
Closed

Uploading crashes the application #48

ghost opened this issue Jan 10, 2019 · 10 comments

Comments

@ghost
Copy link

ghost commented Jan 10, 2019

buffer.js:627
    return _copy(this, target, targetStart, sourceStart, sourceEnd);
           ^

TypeError: argument should be a Buffer
    at Buffer.copy (buffer.js:627:12)
    at Stream.source.on.data (/var/www/removed/node_modules/megajs/dist/main.node-cjs.js:1837:12)
    at Stream.emit (events.js:185:15)
    at Stream.reemit (/var/www/removed/node_modules/duplexer/index.js:70:25)
    at Stream.emit (events.js:180:13)
    at Stream.write (/var/www/removed/node_modules/megajs/dist/main.node-cjs.js:346:10)
    at Stream.stream.write (/var/www/removed/node_modules/through/index.js:26:11)
    at Stream.ondata (internal/streams/legacy.js:15:31)
    at Stream.emit (events.js:180:13)
    at Stream.<anonymous> (/var/www/removed/node_modules/megajs/dist/main.node-cjs.js:53:20)

@qgustavor
Copy link
Owner

How are you uploading files? folder.upload(name, buffer, callback) or stream = folder.upload(name); stream.write(buffer). Can you simplify your code and publish a simple example where it fails? You can use Runkit, Repl.it or something similar.

@ghost
Copy link
Author

ghost commented Jan 10, 2019

I use Multer (https://github.com/expressjs/multer)

class MegaStorage {
	constructor(opts) {
		this.opts = opts;
	}

	_handleFile(req, file, cb) {
		if(!this.opts.email) return cb(new Error('no email'));
		if(!this.opts.password) return cb(new Error('no password'));

		let mega = require('megajs');
		mega(this.opts, (err, storage) => {
			if(err) return cb(err);

			file.stream.pipe(storage.root.upload(file.originalname, (err, file) => {
				if(err) return cb(err);
				file.link((err, link) => {
					if(err) return cb(err);
					cb(null, {
						url: link
					});
				});
			}));
		});
	}
	
	_removeFile(req, file, cb) {
		console.log("remove");
		// TODO: Add remove file
	}
}

module.exports = function(opts) {
	return new MegaStorage(opts);
}

@qgustavor
Copy link
Owner

qgustavor commented Jan 10, 2019

Is it something like this? https://repl.it/repls/WoefulQuestionableMicrostation

Where in Multer you're getting file? As you can see file.stream is undefined in files returned by it. Make sure the stream you're reading returns buffers, not files or other kind of data. If needed convert those streams to buffer streams.

Edit: Now I noticed you're using Storage Engine. I will edit the repl.

Edit 2: I got Multer working and it's using Buffer streams (not File streams, like gulp object streams). Turns that your code is similar to this test.

The second line on your call stack is this line on the source. For some reason the "data" event is being called before the input stream is resumed by handleChunk, so at this point uploadBuffer is still not initialized.

Try to use buffers, streams are hard (but if you can find a better solution, you're welcome).

@ghost
Copy link
Author

ghost commented Jan 10, 2019

Okay thanks i will test a bit around.

@qgustavor
Copy link
Owner

qgustavor commented Jan 10, 2019

I remembered something: you're using streams and you have the file size. Pass the file size to the upload function so it will not need to buffer the entire input stream to guess the file size.

- storage.root.upload(file.originalname, (err, file) => {
+ storage.root.upload({
+    name: file.originalname,
+    size: file.size
+ }, (err, file) => {

@ghost
Copy link
Author

ghost commented Jan 11, 2019

@qgustavor
Copy link
Owner

I noticed some things:

  • Multer don't set the file size on the _handleFile of custom storages, so file.size will be undefined and it will need to buffer all input before uploading.
  • I tried using a small file and it worked; tried using a 10 MB file and it failed once; tried again and it worked, tried again and it failed: it's random.
  • You used passwordsgenerator.net, a website, when you could use just openssl rand -hex 16... but well, that will not solve this issue.

I can be a issue on the upload chunking code. It's hard: tonistiigi's code uploads files using a single connection, but seems MEGA don't supports that anymore. I implemented chunking to fix #13 and #43.

Seems this code is bugged when handling streams like those from request. Larger the uploaded file then higher the chance of triggering the bug. I don't know what exactly is causing that. Can someone check this code?

@qgustavor
Copy link
Owner

How about replacing request? We can use isomorphic-fetch. As now uploading and downloading are chunked by default we don't need to use ReadableStreams to reduce memory usage.

@qgustavor
Copy link
Owner

I checked other libraries: superagent is 6.2 kB, axios is 4.3 kB, isomorphic-fetch is 2.7 kB, but those require a promise polyfill, so we need to add an extra 2.4 kB.

The current "request-like" module is way lighter than those in browser. In the other hand in Node the request library is quite heavy and maybe is the source of those issues.

Seems most people use the Node build, then we can do the following: update the current code to use isomorphic-fetch and add es6-promise to the browser version. If raw fetch gets complicated then we can switch to axios, but I don't think that's a problem as there's already some abstractions in api.js.

@ghost ghost closed this as completed Sep 21, 2019
@qgustavor
Copy link
Owner

Is it working for you now?

@qgustavor qgustavor unpinned this issue Sep 21, 2019
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant