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

Test that postMessage doesn't block on event loop #23270

Merged
merged 5 commits into from
Apr 28, 2020

Conversation

RReverser
Copy link
Contributor

Per spec postMessage should queue a message up on the target port immediately.

Instead, when Workers are involved, some implementations queue a task up on current thread's event loop, which means that in case it gets blocked, some messages are never sent.

Resolves whatwg/html#5485.

@RReverser
Copy link
Contributor Author

RReverser commented Apr 27, 2020

cc @annevk @surma - I want to convert this to .any.js later, but that's currently blocked on #23268, and I don't want to just manually duplicate test code.

I think that in the current form this is good enough to verify the expected behaviour and will help me as a reference for Chromium bug report.

I've also verified that this works as expected on Firefox nightly (which has COOP support by default).

Per spec postMessage should queue a message up on the target port immediately.

Instead, when Workers are involved, some implementations queue a task up on current thread's event loop, which means that in case it gets blocked, some messages are never sent.

Resolves whatwg/html#5485.
@annevk
Copy link
Member

annevk commented Apr 27, 2020

It seems like you haven't seen whatwg/html#5476. We should not rely on data URL dedicated workers being able to share memory in this way.

Looks good otherwise. Might wanna use more const though.

@RReverser
Copy link
Contributor Author

Yeah definitely didn't see it, just searched the codebase for Workers, noticed that others also used data-URI and assumed it's fine.

I can convert, but not sure where to put the worker JS. My understanding is that I can't name it as .worker.js because it will be assumed to be a separate test, but just .js should be fine?

@surma
Copy link
Contributor

surma commented Apr 27, 2020

Could just create a blob URL

@RReverser
Copy link
Contributor Author

Ah yeah I suppose I could. For now already updated to use a separate file, but open to changing it to something else if preferable.

@guest271314
Copy link
Contributor

When the test


var w = new Worker(`data:text/javascript,
  postMessage(null);
  onmessage = e => Atomics.store(e.data, 0, 1);
`);

w.onmessage = _ => {
  var a = new Int32Array(new SharedArrayBuffer(4));
  w.postMessage(a);
  while(Atomics.load(a, 0) === 0);
  console.log(Atomics.load(a, 0), 1);
};

is run at Nightly 77 at console at this page

Content Security Policy: The page’s settings blocked the loading of a resource at data:text/javascript, postMessage(null)… (“script-src”).

run at plnkr.co and file: protocol

DataCloneError: Worker.postMessage: The SharedArrayBuffer object cannot be serialized. The Cross-Origin-Opener-Policy and Cross-Origin-Embedder-Policy HTTP headers can be used to enable this

@guest271314
Copy link
Contributor

Can a version be made without SharedArrayBuffer and Atomics for the ability to test locally at file: protocol and avoid the error described above?

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

This looks good to me, but please put the helper file in a resources/ directory.

workers/postMessage_block_worker.js.headers Outdated Show resolved Hide resolved
@RReverser
Copy link
Contributor Author

@guest271314 As the error says, you need to have corresponding headers set for SAB to work. They're set in these tests correctly, but won't be on 3rd-party resource.

I don't think version without SAB is possible, because it's the only Web API that provides synchronous communication channel that doesn't rely on the event loop, and making sure that we don't wait for an event loop spin is the primary point of this test.

@RReverser
Copy link
Contributor Author

@guest271314 If you really need to run this test on a different hosting / protocol, you can enable shared memory on any pages without COOP/COEP via about:config, although obviously this is not recommended.

@RReverser
Copy link
Contributor Author

This looks good to me, but please put the helper file in a resources/ directory.

@annevk The workers folder doesn't seem to have resources subfolder. I can create one, but it seems that this role is currently served by support folder instead?

Should I still create a new resources folder or put files in support instead?

@RReverser
Copy link
Contributor Author

Moved to support for now, let me know if you think this should be changed.

Copy link
Member

@annevk annevk left a comment

Choose a reason for hiding this comment

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

Thanks @RReverser! I take it this passes in Firefox and fails in Chrome? Please list the Chrome bug here in a comment in case someone needs it while going through blame.

@annevk annevk merged commit 3320205 into web-platform-tests:master Apr 28, 2020
@RReverser
Copy link
Contributor Author

@annevk Thanks for the review! The relevant Chromium bug is here: https://bugs.chromium.org/p/chromium/issues/detail?id=1075645

@RReverser RReverser deleted the test-postmessage-block branch April 28, 2020 12:03
@guest271314
Copy link
Contributor

@guest271314 As the error says, you need to have corresponding headers set for SAB to work. They're set in these tests correctly, but won't be on 3rd-party resource.

I don't think version without SAB is possible, because it's the only Web API that provides synchronous communication channel that doesn't rely on the event loop, and making sure that we don't wait for an event loop spin is the primary point of this test.

Am not entirely sure what the test does and what the expected result is, after reading the HTML issue, where it was mentioned that the test can be composed without using SharedArrayBuffer

whatwg/html#5485 (comment)

While the example by @RReverser is written using SABs and Atomics, they are not required to make this an observable interop issue.

Yeah they're here just for visibility; I could use something as simple as console.log (that's what I did initially) but it has own issues in Firefox because it can't print anything while main thread is locked.

Hence, I think Atomics make a better base for a WPT.

Is the above statement not true and correct?

That using SharedArrayBuffer for the test was a choice?

Does the same issue occur when transferring a ReadableStream, WritableStream or TransformStream at postMessage() (https://bugs.chromium.org/p/chromium/issues/detail?id=894838; https://bugs.chromium.org/p/chromium/issues/detail?id=910471)?

For context, when initially read the title of the PR had the impression that the issues that had encountered transferring 500,000 TypedArrays using postMessage() from Worker to AudioWorklet WebAudio/web-audio-api-v2#73 (comment) and the results thereof (impact on blocking audio processing and output leading to periodic gaps of silence or glitches) at both Chromium and Firefox would be covered, however that does not appear to be the case. This test is not immediately clear what the expected result is and covers only the case of a single SharedArrayBuffer, not voluminous messages being posted over an extended period of time.

@guest271314
Copy link
Contributor

@RReverser It could probably be argued that the gaps in silence when using MessagePort.postMessage() to transfer hundreds of thousands of TypedArrays or posting a single ReadableStream to AudioWorklet are an AudioWorklet issue, yet if in fact the transfers involving Worker and postMessage() are within the scope of the issue you raised as to the expectation of non-blocking, should apply in AudioWorkletGlobalScope and AudioWorkletProcessor and should be non-blocking in that context as well?

plnkr to observe the output and compare for yourself using MessagePort.postMessage() transferring N messages (potentially an infinite input stream) https://plnkr.co/edit/yh5A66UhMnlpq0JF?preview and transferring a single ReadableStream from Worker https://plnkr.co/edit/nECtUZ.

@RReverser
Copy link
Contributor Author

Is the above statement not true and correct?

What I mentioned there is that issue can be observed in Chrome even without SAB, but e.g. console.log won't work in Firefox. In the end, SAB is the only API that can be used for automation, and it was the original use-case that revealed this issue.

You still haven't explained why you want to use a 3rd-party hosting for this WPT test and whether the provided workaround works for you, so I'm not sure how to help further at this point.

For context, when initially read the title of the PR had the impression that the issues that had encountered transferring 500,000 TypedArrays using postMessage() from Worker to AudioWorklet WebAudio/web-audio-api-v2#73 (comment) and the results thereof (impact on blocking audio processing and output leading to periodic gaps of silence or glitches) at both Chromium and Firefox would be covered, however that does not appear to be the case. This test is not immediately clear what the expected result is and covers only the case of a single SharedArrayBuffer, not voluminous messages being posted over an extended period of time.

Sorry if I misunderstand, but it seems like you're describing completely different issue - a performance overhead from sending large amount of messages in Chromium?

If so, it's not related to this test in any way which checks for a cross-browser spec compliance in regards to sending a single message.

If you have a well-defined repro and believe this is a bug, you might want to submit an issue on the Chromium repo instead.

@guest271314
Copy link
Contributor

Consider

WebAudio/web-audio-api-v2#73 (comment)

I think the guiding principle for AudioWorkletNode and process() is that it should focus only doing signal processing converting the input(s) (and parameters) into some appropriate ouput(s). postMessage is provided because we knew there would need to be occasional communication between the main thread and the audio thread.

In particular, if postMessge() - whether from Worker or main thread, is in fact expected to be "non-blocking" then it should not matter if 1 or 1,000,000 messages are posted to AudioWorkletGlobalScope, the result should be non-blocking as to any other ongoing processes.

Both inferred or implied recognition that using postMessage() can block and the concept that postMessage() should not block are inconsistent. Why theory is accurate based on the evidence of actual reproducing postMessage() calls?

@guest271314
Copy link
Contributor

You still haven't explained why you want to use a 3rd-party hosting for this WPT test and whether the provided workaround works for you, so I'm not sure how to help further at this point

Perform all WPT tests by hand, locally, here. No automation.

Already filed Firefox and Chromium bugs - and for the case of Disable cache further impacting whatever code is ongoing.

Again, the title of the test implies full coverage of various cases where postMessage() is expected to be non-blocking, correct?

If that is the case postMessage() should be non-blocking in AudioWorkletGlobalScope.

What is the difference between transferring a ReadableStream from Worker and using SharedArrayBuffer?

In any event, at least this is a start in the same or similar domain.

@RReverser
Copy link
Contributor Author

Sorry, I'm not familiar with Web Audio API, and, as I said above, I think it's best you raise a separate issue (or a PR if you have something specific in mind), so that people familiar with it could review.

This PR has been already closed and might be not the best place for such discussions.

@RReverser
Copy link
Contributor Author

Perform all WPT tests by hand, locally, here. No automation.

That's probably the only part I can respond to - if you haven't seen yet, there is a doc section on running WPT tests locally: https://web-platform-tests.org/running-tests/from-local-system.html

You can't just extract the code from the test to a different server without replicating corresponding configuration, as it won't behave in the same way (as you've seen for yourself).

@guest271314
Copy link
Contributor

@RReverser

What happens when the automation testing code has a bug? Then there are two code bases being tested. Prefer to just extract the relevant code and run locally. Am not sure how the tests at #22779 are passing where when ran locally loadedmetadata never fires due to the track from canvas.captureStream() never unmuting without other code being run, and the pixel only matched at best 10% of the time. Automation can provide inaccurate results.

Have other experiments and proposals that am considering re extending MessagePort and postMessage().

Again, your PR at least breaches the topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Clarify sync vs async nature of Worker::postMessage
6 participants