feat(extensions): Broadcast Channel API #10527

crowlKats · 2021-05-07T13:06:14Z

Current approach is wrong and doesn't work

bnoordhuis

I'm getting BorrowMutErrors when I try to postMessage. I'll investigate.

thread 'main' panicked at 'already borrowed: BorrowMutError', runtime/metrics.rs:189:35

extensions/broadcast_channel/01_broadcast_channel.js

extensions/broadcast_channel/lib.rs

bnoordhuis

@lucacasonato This is ready for review. I've left some comments.

bnoordhuis · 2021-05-16T14:31:40Z

cli/tests/integration_tests.rs

+      .arg("--location")
+      .arg("http://127.0.0.1/")
+      .arg("--allow-read")
+      .arg("--no-check") // TS typechecking makes the test hang.


Because it tries to typecheck the inline script.

bnoordhuis · 2021-05-16T14:34:53Z

extensions/broadcast_channel/01_broadcast_channel.js

+  // Defer to avoid starving the event loop. Not using queueMicrotask()
+  // for that reason: it lets promises make forward progress but can
+  // still starve other parts of the event loop.


I observe that with queueMicrotask() e.g. setTimeout() callbacks don't run when ping-ponging messages over a BroadcastChannel.

extensions/web/02_event.js

tools/wpt.ts

lucacasonato

As always, implementation is very clean. Nice work. Mostly just some nitpicks.

I do want to voice some concerns regarding how the resources work. Currently there is one broadcast channel resource for all BroadcastChannels in an isolate. This is unfortunate because it makes the resource sanitizer in tests less effective. Say you create a BC outside a test, then another one in a test, and you don't close the one in the test, then there will be no resource leakage warning. I assume this approach is a lot faster though, because less boundary crossings, so this isn't a blocker for me. Logically one resource per BroadcastChannel would make more sense for me though.

extensions/broadcast_channel/01_broadcast_channel.js

lucacasonato · 2021-05-16T22:28:16Z

extensions/broadcast_channel/in_memory_broadcast_channel.rs

@@ -0,0 +1,97 @@
+// Copyright 2018-2021 the Deno authors. All rights reserved. MIT license.


What about feature flagging this entire file, and enabling it by default?

extensions/broadcast_channel/lib.rs

extensions/web/02_event.js

tools/wpt.ts

extensions/broadcast_channel/lib.deno_broadcast_channel.d.ts

bnoordhuis · 2021-05-17T08:23:38Z

Currently there is one broadcast channel resource for all BroadcastChannels in an isolate.

It's for reasons of correctness, not performance - that's just a happy accident. :-)

WPT requires that messages are delivered to receivers in subscription order but tokio::sync::broadcast doesn't make that guarantee (and indeed doesn't seem to work that way.)

If you feel really strongly, I could implement a tokio::sync::broadcast with stronger ordering guarantees, but that means more code and complexity.

lucacasonato · 2021-05-17T08:29:54Z

If you feel really strongly, I could implement a tokio::sync::broadcast with stronger ordering guarantees, but that means more code and complexity.

Nope, with this background info it makes sense now. Lets leave it as is.

bnoordhuis · 2021-05-18T13:49:57Z

@lucacasonato How do I get the linter to pass? It's using an existing deno binary that doesn't know about the new BroadcastChannel global... Catch-22.

lucacasonato · 2021-05-18T15:24:59Z

@bnoordhuis Fixed now through merging #10652. Just merge with main / rebase

bnoordhuis · 2021-05-20T07:27:23Z

The integration test appears to be hanging on the CI but I can't reproduce locally, it always passes (and in less than a second):

2021-05-19T22:28:52.2896211Z test integration::broadcast_channel has been running for over 60 seconds

I've added an #[ignore] to see if it fares better.

bnoordhuis · 2021-05-20T09:13:30Z

All green with the integration test disabled.

@lucacasonato Ideas on how to move forward? I don't mind investigating further but I don't have any leads and I'd like to get this merged before the inevitable next round of merge conflicts.

lucacasonato · 2021-05-20T09:54:06Z

cli/tests/integration_tests.rs

@@ -2715,6 +2715,25 @@ console.log("finish");
    assert!(end - start < Duration::new(10, 0));
  }

+  #[test]
+  #[ignore] // Hangs on CI, passes locally.
+  fn broadcast_channel() {


So this is a integration test instead of a JS unit test because it requires --ignore?

Because it requires --location.

Oh... don't we run unit tests with --location? If not, @bartlomieju any objections to doing that?

We do:

--location=http://js-unit-tests/foo/bar

Oh, I remember the other issue: the test hangs when run as a unit test because deno test tries to type-check the embedded script. I guess I'll move it to a separate file.

@bnoordhuis I can get the test to pass without --no-check.

lucacasonato · 2021-05-20T10:51:31Z

I will take a quick look and see if I can get it to fail / pass, but if not let's merge it and investigate later.

caspervonb · 2021-05-20T11:00:49Z

Deadlock somewhere? haven't read through the implementation yet but test hangs for me.

lucacasonato · 2021-05-20T11:02:33Z

Found it - wrong assumption about worker startup being sync.

bnoordhuis · 2021-05-20T11:18:27Z

Thanks for helping debug. I've turned it into a unit test and Luca pointed out how to fix the race.

lucacasonato · 2021-05-20T18:36:39Z

extensions/broadcast_channel/01_broadcast_channel.js

+    constructor(name) {
+      super();
+
+      window.location;


This should be done in Rust.

lucacasonato · 2021-05-20T19:08:25Z

Tried it out locally, and seems to work well. I have just two concerns left to address now:

this should be --unstable until we decide about cross process or not
the --location check and origin namespacing should happen generically in rust, because other implementers might not have a window origin

Co-Authored-By: Ben Noordhuis <[email protected]> Fixes: denoland#10354

bnoordhuis · 2021-05-22T17:02:34Z

I've added a --unstable check in op_broadcast_subscribe(). That's the main entry point.

Since it's behind --unstable now and because I don't want to think about --location too much, I've simply ripped out the window.location check.

Replaces the file-backed provider by an in-memory one because proper file locking is a hard problem that detracts from the proof of concept. Teach the WPT runner how to extract tests from .html files because all the relevant tests in test_util/wpt/webmessaging/broadcastchannel are inside basics.html and interface.html.

lucacasonato

LGTM - the origin handling is incorrect (as shown by the broken WPT), but that is fine for now. We will need to revisit it when we do cross process BroadcastChannel.

Thanks for sitting through this @bnoordhuis :-)

bnoordhuis · 2021-05-24T10:41:07Z

Sorry @crowlKats, something went wrong while merging. I used rebase-without-squash with the intent of keeping you as the author of the first commit but for some reason I show up as the author. Mea culpa.

bnoordhuis force-pushed the broadcast_channel branch from c250184 to f538efe Compare May 8, 2021 11:00

bnoordhuis reviewed May 8, 2021

View reviewed changes

extensions/broadcast_channel/01_broadcast_channel.js Outdated Show resolved Hide resolved

bnoordhuis force-pushed the broadcast_channel branch from f538efe to f4ccb55 Compare May 8, 2021 23:06

lucacasonato reviewed May 8, 2021

View reviewed changes

extensions/broadcast_channel/lib.rs Outdated Show resolved Hide resolved

bnoordhuis force-pushed the broadcast_channel branch 3 times, most recently from c71d60a to c220bcd Compare May 15, 2021 18:45

bnoordhuis force-pushed the broadcast_channel branch 2 times, most recently from 7735044 to 8fc6262 Compare May 16, 2021 14:29

bnoordhuis reviewed May 16, 2021

View reviewed changes

lucacasonato reviewed May 16, 2021

View reviewed changes

bnoordhuis force-pushed the broadcast_channel branch from 4419d72 to e9297ce Compare May 18, 2021 13:45

lucacasonato closed this May 18, 2021

lucacasonato reopened this May 18, 2021

bnoordhuis force-pushed the broadcast_channel branch from e9297ce to 37dc0c8 Compare May 18, 2021 18:32

lucacasonato mentioned this pull request May 19, 2021

Web Platform Tests coverage tracking issue #9001

Closed

28 tasks

bnoordhuis force-pushed the broadcast_channel branch from 37dc0c8 to c9861ca Compare May 19, 2021 21:51

lucacasonato reviewed May 20, 2021

View reviewed changes

bnoordhuis force-pushed the broadcast_channel branch from 5b4b110 to f390896 Compare May 20, 2021 11:17

lucacasonato reviewed May 20, 2021

View reviewed changes

bnoordhuis added 2 commits May 22, 2021 18:08

chore: update wpt

5faa9da

feat(extensions): add BroadcastChannel

33e7873

Co-Authored-By: Ben Noordhuis <[email protected]> Fixes: denoland#10354

bnoordhuis force-pushed the broadcast_channel branch from f390896 to a782b61 Compare May 22, 2021 17:00

bnoordhuis force-pushed the broadcast_channel branch from a782b61 to c1f9393 Compare May 22, 2021 17:35

lucacasonato approved these changes May 23, 2021

View reviewed changes

bnoordhuis merged commit af15463 into denoland:main May 23, 2021

crowlKats deleted the broadcast_channel branch September 29, 2021 21:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(extensions): Broadcast Channel API #10527

feat(extensions): Broadcast Channel API #10527

crowlKats commented May 7, 2021

bnoordhuis left a comment

bnoordhuis left a comment

bnoordhuis May 16, 2021

bnoordhuis May 16, 2021

lucacasonato left a comment

lucacasonato May 16, 2021

bnoordhuis commented May 17, 2021

lucacasonato commented May 17, 2021

bnoordhuis commented May 18, 2021

lucacasonato commented May 18, 2021 •

edited

Loading

bnoordhuis commented May 20, 2021

bnoordhuis commented May 20, 2021

lucacasonato May 20, 2021

bnoordhuis May 20, 2021

lucacasonato May 20, 2021

caspervonb May 20, 2021

bnoordhuis May 20, 2021

lucacasonato May 20, 2021

lucacasonato commented May 20, 2021

caspervonb commented May 20, 2021 •

edited

Loading

lucacasonato commented May 20, 2021

bnoordhuis commented May 20, 2021

lucacasonato May 20, 2021

lucacasonato commented May 20, 2021 •

edited

Loading

bnoordhuis commented May 22, 2021

lucacasonato left a comment

bnoordhuis commented May 24, 2021

		@@ -0,0 +1,97 @@
		// Copyright 2018-2021 the Deno authors. All rights reserved. MIT license.

feat(extensions): Broadcast Channel API #10527

feat(extensions): Broadcast Channel API #10527

Conversation

crowlKats commented May 7, 2021

bnoordhuis left a comment

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucacasonato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bnoordhuis commented May 17, 2021

lucacasonato commented May 17, 2021

bnoordhuis commented May 18, 2021

lucacasonato commented May 18, 2021 • edited Loading

bnoordhuis commented May 20, 2021

bnoordhuis commented May 20, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucacasonato commented May 20, 2021

caspervonb commented May 20, 2021 • edited Loading

lucacasonato commented May 20, 2021

bnoordhuis commented May 20, 2021

Choose a reason for hiding this comment

lucacasonato commented May 20, 2021 • edited Loading

bnoordhuis commented May 22, 2021

lucacasonato left a comment

Choose a reason for hiding this comment

bnoordhuis commented May 24, 2021

lucacasonato commented May 18, 2021 •

edited

Loading

caspervonb commented May 20, 2021 •

edited

Loading

lucacasonato commented May 20, 2021 •

edited

Loading