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

Support asynchronous read-file function (asyncReadFileFn) #304

Closed
wants to merge 5 commits into from

Conversation

drzraf
Copy link
Collaborator

@drzraf drzraf commented Jul 30, 2020

See #295, #296 and #300

@AidasK
Copy link
Member

AidasK commented Jul 30, 2020

Can you also check that async is compiled in dist/ folder to ES5 via grunt? This way you could use promises everywhere. Though you might need to update examples and docs due to a breaking changes

@drzraf
Copy link
Collaborator Author

drzraf commented Jul 30, 2020

  • Not sure I understood. I committed the result of grunt template too. Something else?
  • The update to karma-coverage v2, needed for async support does not seem to play well with the rest of Travis, but the testsuite is green on my local system. Any hint?

@AidasK
Copy link
Member

AidasK commented Jul 31, 2020

The thing is that grunt does not compile files to es5, that means that browsers are going to crash if they do not support async syntax. That is why we need to update grunt minify method to recompile file to es5.

@AidasK
Copy link
Member

AidasK commented Aug 29, 2020

Is this ready to be merged?

@drzraf
Copy link
Collaborator Author

drzraf commented Sep 3, 2020

No, I'm revamping the PR because the way I checked for asynchronicity of function is not compatible with bundled code.
(I'll provide a test too).
#309 is a prerequisite as this brings in a transpilation toolchain and testing transpiled code instead of sourcecode.

@drzraf drzraf changed the title support async function for readFileFn [WIP] support async function for readFileFn Sep 3, 2020
@drzraf drzraf changed the base branch from master to v3 September 12, 2020 02:45
@drzraf drzraf changed the title [WIP] support async function for readFileFn Support asynchronous read-file function (asyncReadFileFn) Sep 12, 2020
@drzraf drzraf requested a review from AidasK September 12, 2020 02:51
src/flow.js Show resolved Hide resolved
xhr.restore();
});

it('synchronous initFileFn and asyncReadFileFn', async function (done) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

travis build does not pass 1 test

Copy link
Collaborator Author

@drzraf drzraf Sep 15, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't get why.
It's probably an error parsing the JS code of the test, but what exactly? (https://caniuse.com/?compare=firefox+60,firefox+80&compareCats=JS):

  • backticks
  • async
  • window.crypto.subtle.digest
  • Uint8Array
  • TextEncoder

Still, I guess all of these are supported by FF 60...

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried SauceLabs locally, but get errors:

15 09 2020 12:31:46.912:INFO [launcher]: Starting browser firefox 45 (Linux) on SauceLabs
2020-09-15T15:32:43.192Z ERROR webdriver: Request failed with status 500 due to Error: Infrastructure Error -- The Sauce VMs failed to start the browser or device.
For more info, please check https://wiki.saucelabs.com/display/DOCS/Common+Error+Messages
2020-09-15T15:32:43.196Z ERROR webdriver: Error: Infrastructure Error -- The Sauce VMs failed to start the browser or device.
For more info, please check https://wiki.saucelabs.com/display/DOCS/Common+Error+Messages
    at getErrorFromResponseBody (flow.js/node_modules/webdriver/build/utils.js:121:10)
    at WebDriverRequest._request (/flow.js/node_modules/webdriver/build/request.js:149:56)
    at process._tickCallback (internal/process/next_tick.js:68:7)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried running it on the latest firefox? If it works, it might be just a quirk on saucelabs

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

karma start karma.conf.js --browsers=sl_firefox_2 (actually modified it for FF45)

16 09 2020 11:22:33.219:DEBUG [launcher.SauceConnect]: 16 Sep 11:22:33 - Sauce Connect is up, you may start your tests.
16 09 2020 11:23:09.815:WARN [launcher]: firefox 45 (Linux) on SauceLabs have not captured in 60000 ms, killing.
16 09 2020 11:23:09.816:ERROR [SaucelabsLauncher]: Could not quit the Saucelabs selenium connection. Failure message:
16 09 2020 11:23:09.816:ERROR [SaucelabsLauncher]: TypeError: Cannot read property 'deleteSession' of null

I don't know what am I doing wrong.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the issue is related to karma-runner/karma-sauce-launcher#210
Could you please investigate whether SauceLabs backend is correctly configured?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doing:
SAUCE_USERNAME=flowjs SAUCE_ACCESS_KEY=53e609a9-cb5d-4eac-a888-xxx grunt karma:saucelabs
I still can't test saucelabs and can't explain why the CI fails.

I know how annoying to get an always-red CI and would prefer to avoid this.
Any hope you could take the time to double check the SauceLabs configuration? (and the travis' one?)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you stopped the CI ?

@drzraf
Copy link
Collaborator Author

drzraf commented Sep 14, 2020

Note: I observed problems with progress event reporting using test/FakeXMLHttpRequestUpload.js (see #313)

@drzraf drzraf force-pushed the asyncReadFileFn branch 3 times, most recently from 0a0e1bb to 93f9129 Compare September 15, 2020 14:22
this.readStreamState.resolve();

// Equivalent to readFinished()
this.readState = 2;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are releasing v3 version, maybe it's simpler to make some breaking changes? We can get rid off all those semaphores :))

Copy link
Collaborator Author

@drzraf drzraf Sep 16, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree Deferred-based locking could benefit to the rest of the codebase, but could we postpone that to a later commit/PR, otherwise this will significantly broaden the scope of the PR and make it harder to review/test/revert.

src/flow.js Outdated Show resolved Hide resolved
@drzraf drzraf force-pushed the asyncReadFileFn branch 2 times, most recently from 5393f12 to 2b3246c Compare September 16, 2020 14:20
@drzraf
Copy link
Collaborator Author

drzraf commented Sep 16, 2020

Added more tests about chunk size mismatch.
I ran a couple of random tests, all succeeded.

Test File is 36 bytes long. Now uploads 18 simultaneous chunks of at most 7 bytes
Test File is 31 bytes long. Now uploads 6 simultaneous chunks of at most 8 bytes
Test File is 7 bytes long. Now uploads 15 simultaneous chunks of at most 11 bytes
Test File is 66 bytes long. Now uploads 19 simultaneous chunks of at most 16 bytes
Test File is 29 bytes long. Now uploads 16 simultaneous chunks of at most 13 bytes
Test File is 84 bytes long. Now uploads 14 simultaneous chunks of at most 2 bytes
Test File is 49 bytes long. Now uploads 2 simultaneous chunks of at most 24 bytes
Test File is 24 bytes long. Now uploads 20 simultaneous chunks of at most 7 bytes
Test File is 41 bytes long. Now uploads 13 simultaneous chunks of at most 28 bytes
Test File is 17 bytes long. Now uploads 17 simultaneous chunks of at most 5 bytes
Test File is 21 bytes long. Now uploads 11 simultaneous chunks of at most 2 bytes
Test File is 16 bytes long. Now uploads 15 simultaneous chunks of at most 3 bytes
Test File is 67 bytes long. Now uploads 15 simultaneous chunks of at most 24 bytes
Test File is 65 bytes long. Now uploads 6 simultaneous chunks of at most 10 bytes
Test File is 14 bytes long. Now uploads 14 simultaneous chunks of at most 2 bytes
Test File is 83 bytes long. Now uploads 18 simultaneous chunks of at most 10 bytes
Test File is 76 bytes long. Now uploads 1 simultaneous chunks of at most 3 bytes
Test File is 2 bytes long. Now uploads 11 simultaneous chunks of at most 4 bytes
Test File is 34 bytes long. Now uploads 5 simultaneous chunks of at most 2 bytes
Test File is 87 bytes long. Now uploads 15 simultaneous chunks of at most 8 bytes
Test File is 16 bytes long. Now uploads 9 simultaneous chunks of at most 2 bytes
Test File is 23 bytes long. Now uploads 13 simultaneous chunks of at most 5 bytes
Test File is 91 bytes long. Now uploads 1 simultaneous chunks of at most 5 bytes
Test File is 69 bytes long. Now uploads 11 simultaneous chunks of at most 4 bytes
Test File is 40 bytes long. Now uploads 1 simultaneous chunks of at most 6 bytes
Test File is 54 bytes long. Now uploads 6 simultaneous chunks of at most 9 bytes
Test File is 31 bytes long. Now uploads 8 simultaneous chunks of at most 13 bytes
Test File is 15 bytes long. Now uploads 2 simultaneous chunks of at most 1 bytes
Test File is 42 bytes long. Now uploads 10 simultaneous chunks of at most 11 bytes
Test File is 40 bytes long. Now uploads 1 simultaneous chunks of at most 25 bytes

Raphaël Droz added 2 commits October 12, 2020 23:16
…total amount of bytes to upload.

The number of chunks may also be off. Let fail safely:
- if read using an async function
- and the last chunk read less bytes than usual (including 0)
- and the current chunk read no byte
- assume that this chunk is in fact superfluous and simulate its completion.

Previous discussion on:
flowjs#42
flowjs#318

In the long-term Flow.js may have to support the concept of "approximate chunk number".
@drzraf
Copy link
Collaborator Author

drzraf commented Oct 15, 2020

Closing in favor of #321

@drzraf drzraf closed this Oct 15, 2020
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