S3 Signature on Worker #1702

fromkeith · 2016-12-16T01:02:09Z

Brief description of the changes [REQUIRED]

Have the option for the S3 Signing process (v4) to be done in a worker thread. My browser no longer freezes on large files.

What browsers and operating systems have you tested these changes on? [REQUIRED]

Windows 10

Chrome 56.0
Firefox 50.1.0
Edge 38
Internet Explorer 11

OSX

Safari 10.0.2

Are all automated tests passing? [REQUIRED]

Yes.
I haven't build the docs though...

Is this pull request against develop or some other non-master branch? [REQUIRED]

Develop

More Info

You might consider this a scary change :)

A couple things I want to point out, slash get your feedback on.

The worker can be implemented as either Inline or as a separate file.
From what I can find IE is the only one that doesn't support the inline worker.
- So I've added in a graceful fallback to just the old in 'main thread' method.
Some people might not want the extra overhead of the inline file.
- The extra overhead in this case is really the CryptoJS library.
- I've made the option 'useWorker' to be able to specify a string. That way people can override with their own external worker.
The inline worker file is generated at compile time. See the _build-s3-inline-worker build task.
- It's done by concatenating 3 files.
  - worker.start.js, CyrptoJS files, worker.end.js
- A simple script (workerToInline) then converts that file into a string and adds a function to qq.
- We can now include the inline worker as a build file, as per normal js files.
A new build artefact is added for the external worker.
I've added a new S3 output that includes the inline worker. The regular output for s3 should not include it.
Guidance on how you want the build artefacts to look like would be great.

Feedback, changes, clarity, etc.. are all welcome.

to stop a clone error on postMessage.

fromkeith · 2016-12-16T01:04:11Z

Looks like lint failures... fixing that

postMessage and onmessage are defined in the worker scope. we need to set qq on the worker prefix so that crypto has it defined inside the worker scope.

rnicholus

Interesting, and it looks like you put a lot of work into this. I appreciate the effort.

My concern is having yet another JS build artifact. How many bytes does this add to the existing S3 JS build files? Unless the increase is notable, I'd rather just bundle this in with the existing core and UI S3 builds.

Also, I really think unit tests are prudent here, plus some documentation should probably be added to the S3 feature page. I'm happy to assist any way I can, but I'm finding I have less and less time to evolve this library these days.

client/js/s3/request-signer.js

+                    // We will fallback to the inline reader if the worker
+                    // fails to load.
+                    if (options.signatureSpec.useWorker) {
+                        if (!this._worker) {


client/js/s3/request-signer.js

+                                    if (!qq.s3.worker) {
+                                        throw new Error("Missing inline s3 worker.");
+                                    }
+                                    workerUrl = qq.s3.worker();


client/js/s3/request-signer.js

+                                var workerUrl = options.signatureSpec.useWorker;
+                                if (workerUrl === true) {
+                                    if (!qq.s3.worker) {
+                                        throw new Error("Missing inline s3 worker.");


fromkeith · 2016-12-16T20:13:13Z

I never actually looked at the file size differences...

s3-core.min: 142KB
s3-core-worker.min: 150KB

I'll take a look at unit tests and incorporating your code comments a bit later.

rnicholus · 2016-12-17T04:25:19Z

8K difference - after gzipping, that's almost a wash. Using web workers for this task seems like a big win to me, so I'm leaning towards including it in all existing S3 build files. But anything you can do to make the difference even smaller is worthwhile too.

New file to manage the workers. Add unit tests to ensure worker is working. Rename from useWorker to workerUrl.

'qq.s3.worker' func renamed to 'qq.s3.createS3InlineWorkerUrl'. Updated docs for 'useWorker' to 'workerUrl' option change. Propogate options.log to the manager. Get error handling for bad worker urls working. Set 'inline' as the text to set for using our default inline worker.

# Conflicts: # package.json

fromkeith · 2017-01-04T18:03:45Z

From your feedback:

'useWorker' changed to 'workerUrl'
'qq.s3.worker()' changed to 'qq.s3.createS3InlineWorkerUrl()'
Moved the new code from request-signer into a new file 'request-signer.worker-manager.js'
Added in unit tests. I had to fix Date so that it was constant, so I can verify the signatures not changing.
Removed the error from being thrown. Instead it is logged.

fromkeith · 2017-01-04T18:50:25Z

I also want to do a bit more testing on multiple browsers with these changes. I've just done a quick one on Firefox, IE, Edge and Chrome.. not as indepth as I want yet. But I thought you would want to look at the reformatting sooner rather than later.

One thing that I could use guidance on... is that because RequestSigner gets created multiple times, my worker-manager gets created multiple times.. Is there a better place to create the manager, but still get the options from the signer?

rnicholus

Looking good, but a few changes are needed.

client/js/s3/request-signer.js

+                    // not loaded correctly
+                    if (workerManager) {
+                        promise = workerManager.generateSignature(body);
+                        if (promise !== null) {
