search

TTLabs / EvaporateJS

Javascript library for browser to S3 multipart resumable uploads
1.82k stars 207 forks source link

Avoid custom cryptoMd5Method functions to fail silently. This commit … #381

Closed BjoernRuberg closed 7 years ago

BjoernRuberg commented 7 years ago

…uses a try/catch block around the call to that function to achieve that.

Note:
However, using try/catch will reduce execution speed of the md5 creation code what should probably be avoided. Maybe only use this in development mode?
Note:
I directed the error directly to console.error . It might be better to direct it to the configureable error function from the .add() method
BjoernRuberg commented 7 years ago

Alternatively I suggest making cryptoMd5Method async using promises. I just had to change the code locally to make it working with a promise in cryptoMd5Method which uses a webworker to calculate the hash. As this is very CPU intensive work it should not be blocking but working async. Streaming the file chunks in might be even better to avoid the memory usage. Spark md5 supports incremental hashing.

bikeath1337 commented 7 years ago

I have been away dealing with family medical issues. Sorry for the delay.

Evaporate already calls the cryptoMd5Method asynchronously in a Promise. Evaporate just gives examples of how the method could look, at its simplest. If you are aware of an edge case that causes the checkum calc to fail, and if this happens mainly in development, then couldn't you just cater for that edge case in your provided cryptoMd5Method? I agree that the overhead of the try/catch should be avoided and in general, the checksum calculation shouldn't error commonly.

We purposely try to use a Promise to calculate the checksum. If something is preventing it from happening, perhaps you could provide an example and solution? There has been an ongoing discussion about how webworkers execute the checksum as evidenced by lurching progress.

BjoernRuberg commented 7 years ago

The cryptoMd5Method cannot be a promise currently. The event loop is blocked as long as the function is executed. A real promise chain with rejection handling would solve the issue here.

I had my previous work on this deleted in the meantime. However, this is the relevant method in my version which makes it possible to return a promise in cryptoMd5Method. This way you can put the work in a webworker.

https://codepaste.net/57nd24

bikeath1337 commented 7 years ago

Thanks for the feedback. https://github.com/TTLabs/EvaporateJS/blob/master/evaporate.js#L1381 shows that the cryptoMd5Method is called in a proper Promise, which I thought was good enough. If this needs to be built differently, I'd be happy to do that but I would like help understanding why the current implementation wouldn't be executed asynchronously. To be clear, the non-async issue has been reported and not resolved in the issue that initially resulted in support for web workers:

https://github.com/TTLabs/EvaporateJS/issues/308

Are you available to test out branches in an attempt to address this, @BjoernRuberg ?

BjoernRuberg commented 7 years ago

Sorry, I was in holidays. I just plugged in the recent evaporate release in my code and the promise cryptoMd5Method still works. As the source of my problem is gone now and I cannot remember exactly what it was, consider this closed.