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

feat(runtime): Implement classic workers #11338

Merged
merged 28 commits into from
Aug 16, 2021

Conversation

andreubotella
Copy link
Contributor

Part of #10903.

@andreubotella
Copy link
Contributor Author

andreubotella commented Jul 8, 2021

Some of the WPT test failures are panics. Also, I haven't enabled workers/modules/dedicated-worker-import-data-url.any.worker.html because it gets stuck, even though the .any.html test panics.

I'll be enabling the rest of .worker.html tests in a follow-up PR.

@bartlomieju bartlomieju added this to the 1.13.0 milestone Jul 13, 2021
Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

Firstly, this is awesome and I am very happy we will be able to test workers with WPT. Great work. I have some concerns though:

  • It is unfortunate that this requires fetching logic separate from modules. I would rather we use the same file loader as modules (that also supports file://). Doing this would also resolve my second issue:
  • There are no permission checks. Even though this is --unstable with no intent to stabilize, we should try to keep the sandbox and permission system intact.

I am also thinking that maybe we want to add a hidden --enable-testing-features-do-not-use flag to unlock this feature. The flag should also print out a really obnoxious red warning text on startup that can't be silenced with --quiet. This would be akin to the --expose-internals-for-testing in Chromium. @bartlomieju @ry opinions?

@bartlomieju
Copy link
Member

@lucacasonato SGTM, but all the relevant code parts should be commented that they are only exposed in that particular case

@andreubotella
Copy link
Contributor Author

Firstly, this is awesome and I am very happy we will be able to test workers with WPT. Great work. I have some concerns though:

* It is unfortunate that this requires fetching logic separate from modules. I would rather we use the same file loader as modules (that also supports file://). Doing this would also resolve my second issue:

* There are no permission checks. Even though this is `--unstable` with no intent to stabilize, we should try to keep the sandbox and permission system intact.

I initially put this together quickly as a proof of concept, and the sync fetch implementation is all sorts of inefficient (it waits until every script is fetched before running any of them). So I'm fine with changing it to use module fetching logic. However, I'm not too familiar with how module fetching works, and making it usable from runtime doesn't seem too easy on a first glance, especially synchronously. There are also differences in how module imports and importScripts() work – the ones that come to mind are that importScripts() doesn't support import maps, and that we probably don't want it to run TypeScript.

I am also thinking that maybe we want to add a hidden --enable-testing-features-do-not-use flag to unlock this feature. The flag should also print out a really obnoxious red warning text on startup that can't be silenced with --quiet. This would be akin to the --expose-internals-for-testing in Chromium. @bartlomieju @ry opinions?

I'm all for classic workers being implemented behind a hidden flag. That would also help when https://jgraham.github.io/browser-test/ gets adopted by the WHATWG (see whatwg/sg#164).

@andreubotella
Copy link
Contributor Author

While working on getting other worker tests to run, I run into web-platform-tests/wpt#29777, which is not a blocker for this PR, but it is a blocker for enabling worker tests in general until we get #11460 merged.

@bartlomieju
Copy link
Member

Removing the milestone for now, due to upstream issue

@bartlomieju bartlomieju removed this from the 1.13.0 milestone Jul 26, 2021
@bartlomieju bartlomieju added the upstream Changes in upstream are required to solve these issues label Aug 6, 2021
Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

Looking very good. Lets get this landed (even if the WPTs are still blocked on some upstream issues).

runtime/ops/mod.rs Outdated Show resolved Hide resolved
runtime/ops/web_worker/sync_fetch.rs Outdated Show resolved Hide resolved
runtime/ops/worker_host.rs Outdated Show resolved Hide resolved
Comment on lines +16 to +23
// TODO(andreubotella) Properly parse the MIME type
fn mime_type_essence(mime_type: &str) -> String {
let essence = match mime_type.split_once(";") {
Some((essence, _)) => essence,
None => mime_type,
};
essence.trim().to_ascii_lowercase()
}
Copy link
Member

Choose a reason for hiding this comment

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

As this is test only code, I don't particularly care if it doesn't parse mime types correctly. Resolving this TODO is not a blocker for me.

Comment on lines +40 to +41
// TODO(andreubotella) Make the runtime into a resource and add a new op to
// block on each request, so a script can run while the next loads.
Copy link
Member

Choose a reason for hiding this comment

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

Why? Don't we want to block the thread while we are executing "importScripts"?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The thread must block for the duration of the importScripts call, but if multiple arguments are passed, the second script can finish loading while the first is being run, rather than blocking until all scripts are loaded before the first script can run. This optimization might be important in WPT tests, since you have scripts that import other scripts that import other scripts. But we can wait and deal with this only if this becomes a problem, which might never happen.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, that makes sense 👍

Comment on lines +49 to +51
// TODO(andreubotella) It's not good to throw an exception related to blob
// URLs when none of the script URLs use the blob scheme.
// Also, in which contexts are blob URLs not supported?
Copy link
Member

@lucacasonato lucacasonato Aug 14, 2021

Choose a reason for hiding this comment

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

Should always be supported. I'll remove the optional. Not a blocker for this PR tho.

@andreubotella andreubotella marked this pull request as ready for review August 14, 2021 20:39
@andreubotella
Copy link
Contributor Author

Marking as ready for review.

Copy link
Member

@lucacasonato lucacasonato left a comment

Choose a reason for hiding this comment

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

This LGTM now. Thanks @andreubotella. Huge step towards more worker tests. Very exciting!

@ry ry added this to the 1.14.0 milestone Aug 16, 2021
@lucacasonato lucacasonato removed the upstream Changes in upstream are required to solve these issues label Aug 16, 2021
@lucacasonato lucacasonato merged commit ddbb7b8 into denoland:main Aug 16, 2021
@lucacasonato lucacasonato removed this from the 1.14.0 milestone Aug 16, 2021
@andreubotella andreubotella deleted the classic-workers branch August 16, 2021 12:30
andreubotella pushed a commit to andreubotella/deno that referenced this pull request Sep 25, 2021
Classic workers were implemented in denoland#11338, which also enabled the WPT
tests in the `workers` directory. However, the rest of WPT worker tests
were not enabled because a number of them were hanging due to
web-platform-tests/wpt#29777. Now that that WPT issue is fixed, the bulk
of worker tests can be enabled.

There are still a few tests that hang, and so haven't been enabled. In
particular:

- The following tests seem to hang because a promise fails to resolve.
  We can detect such cases in non-worker tests because the process will
  exit without calling the WPT completion callback, but in worker tests
  the worker message ops will keep the event loop running. This will be
  fixed when we add timeouts to WPT tests (denoland#9460).

  - `/fetch/api/basic/error-after-response.any.worker.html`
  - `/html/webappapis/microtask-queuing/queue-microtask-exceptions.any.worker.html`
  - `/webmessaging/message-channels/worker-post-after-close.any.worker.html`
  - `/webmessaging/message-channels/worker.any.worker.html`
  - `/websockets/Create-on-worker-shutdown.any.worker.html`

- The following tests apparently hang because a promise rejection is
  never handled, which will kill the process in the main thread but not
  in workers (denoland#12221).

  - `/streams/readable-streams/async-iterator.any.worker.html`
  - `/workers/interfaces/WorkerUtils/importScripts/report-error-setTimeout-cross-origin.sub.any.worker.html`
  - `/workers/interfaces/WorkerUtils/importScripts/report-error-setTimeout-redirect-to-cross-origin.sub.any.worker.html`
  - `/workers/interfaces/WorkerUtils/importScripts/report-error-setTimeout-same-origin.sub.any.worker.html`
andreubotella pushed a commit to andreubotella/deno that referenced this pull request Oct 7, 2021
Classic workers were implemented in denoland#11338, which also enabled the WPT
tests in the `workers` directory. However, the rest of WPT worker tests
were not enabled because a number of them were hanging due to
web-platform-tests/wpt#29777. Now that that WPT issue is fixed, the bulk
of worker tests can be enabled.

There are still a few tests that hang, and so haven't been enabled. In
particular:

- The following tests seem to hang because a promise fails to resolve.
  We can detect such cases in non-worker tests because the process will
  exit without calling the WPT completion callback, but in worker tests
  the worker message ops will keep the event loop running. This will be
  fixed when we add timeouts to WPT tests (denoland#9460).

  - `/fetch/api/basic/error-after-response.any.worker.html`
  - `/html/webappapis/microtask-queuing/queue-microtask-exceptions.any.worker.html`
  - `/webmessaging/message-channels/worker-post-after-close.any.worker.html`
  - `/webmessaging/message-channels/worker.any.worker.html`
  - `/websockets/Create-on-worker-shutdown.any.worker.html`

- The following tests apparently hang because a promise rejection is
  never handled, which will kill the process in the main thread but not
  in workers (denoland#12221).

  - `/streams/readable-streams/async-iterator.any.worker.html`
  - `/workers/interfaces/WorkerUtils/importScripts/report-error-setTimeout-cross-origin.sub.any.worker.html`
  - `/workers/interfaces/WorkerUtils/importScripts/report-error-setTimeout-redirect-to-cross-origin.sub.any.worker.html`
  - `/workers/interfaces/WorkerUtils/importScripts/report-error-setTimeout-same-origin.sub.any.worker.html`
bartlomieju pushed a commit that referenced this pull request Oct 8, 2021
Classic workers were implemented in #11338, which also enabled the WPT
tests in the `workers` directory. However, the rest of WPT worker tests
were not enabled because a number of them were hanging due to
web-platform-tests/wpt#29777. Now that that WPT issue is fixed, the bulk
of worker tests can be enabled.

There are still a few tests that hang, and so haven't been enabled. In
particular:

- The following tests seem to hang because a promise fails to resolve.
  We can detect such cases in non-worker tests because the process will
  exit without calling the WPT completion callback, but in worker tests
  the worker message ops will keep the event loop running. This will be
  fixed when we add timeouts to WPT tests (#9460).

  - `/fetch/api/basic/error-after-response.any.worker.html`
  - `/html/webappapis/microtask-queuing/queue-microtask-exceptions.any.worker.html`
  - `/webmessaging/message-channels/worker-post-after-close.any.worker.html`
  - `/webmessaging/message-channels/worker.any.worker.html`
  - `/websockets/Create-on-worker-shutdown.any.worker.html`

- The following tests apparently hang because a promise rejection is
  never handled, which will kill the process in the main thread but not
  in workers (#12221).

  - `/streams/readable-streams/async-iterator.any.worker.html`
  - `/workers/interfaces/WorkerUtils/importScripts/report-error-setTimeout-cross-origin.sub.any.worker.html`
  - `/workers/interfaces/WorkerUtils/importScripts/report-error-setTimeout-redirect-to-cross-origin.sub.any.worker.html`
  - `/workers/interfaces/WorkerUtils/importScripts/report-error-setTimeout-same-origin.sub.any.worker.html`
bartlomieju pushed a commit to bartlomieju/deno that referenced this pull request Oct 10, 2021
Classic workers were implemented in denoland#11338, which also enabled the WPT
tests in the `workers` directory. However, the rest of WPT worker tests
were not enabled because a number of them were hanging due to
web-platform-tests/wpt#29777. Now that that WPT issue is fixed, the bulk
of worker tests can be enabled.

There are still a few tests that hang, and so haven't been enabled. In
particular:

- The following tests seem to hang because a promise fails to resolve.
  We can detect such cases in non-worker tests because the process will
  exit without calling the WPT completion callback, but in worker tests
  the worker message ops will keep the event loop running. This will be
  fixed when we add timeouts to WPT tests (denoland#9460).

  - `/fetch/api/basic/error-after-response.any.worker.html`
  - `/html/webappapis/microtask-queuing/queue-microtask-exceptions.any.worker.html`
  - `/webmessaging/message-channels/worker-post-after-close.any.worker.html`
  - `/webmessaging/message-channels/worker.any.worker.html`
  - `/websockets/Create-on-worker-shutdown.any.worker.html`

- The following tests apparently hang because a promise rejection is
  never handled, which will kill the process in the main thread but not
  in workers (denoland#12221).

  - `/streams/readable-streams/async-iterator.any.worker.html`
  - `/workers/interfaces/WorkerUtils/importScripts/report-error-setTimeout-cross-origin.sub.any.worker.html`
  - `/workers/interfaces/WorkerUtils/importScripts/report-error-setTimeout-redirect-to-cross-origin.sub.any.worker.html`
  - `/workers/interfaces/WorkerUtils/importScripts/report-error-setTimeout-same-origin.sub.any.worker.html`
andreubotella pushed a commit to andreubotella/deno that referenced this pull request Oct 25, 2021
The initial implementation of `importScripts()` in denoland#11338 used
`reqwest`'s default client to fetch HTTP scripts, which meant it would
not use certificates or other fetching configuration passed by command
line flags. This change fixes it.

Towards denoland#12472.
bartlomieju pushed a commit that referenced this pull request Oct 27, 2021
…ch` (#12540)

The initial implementation of `importScripts()` in #11338 used
`reqwest`'s default client to fetch HTTP scripts, which meant it would
not use certificates or other fetching configuration passed by command
line flags. This change fixes it.
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.

5 participants