Avoid ReferenceError exceptions if ActionCable is used in a web worker

[rmacklin]

Summary

Before this change, it was not possible to use ActionCable in a web worker because it would throw an error when trying to access window or document, which are not available in the WorkerGlobalScope.

It may be desirable to offload WebSocket interaction to a worker to free up the main thread for other activity. But even in applications where the main thread can handle WebSocket interaction and other tasks just fine, there's still a benefit to running ActionCable in a worker: If it is common for your users to have multiple tabs of your application open at once, you can take advantage of the SharedWorker API to reduce load on your servers. With a SharedWorker, multiple tabs can share a single WebSocket connection, meaning you can drastically reduce the number of active connections (if users commonly have 2 tabs open, this can cut the number of connections in half - if they have more than 2, it's even more significant).

Fortunately, it's not too difficult to address the issues that prevent the current version of ActionCable from running in a worker. To support the worker context in addition to the window context, it's recommended to use self instead of window . So that's what the first change in this PR does. To avoid the document ReferenceError, I've removed the explicit document receiver from the addEventListener and removeEventListener calls. The visibilitychange event won't ever get triggered in a worker, so adding the listener is effectively a no-op there. But this listener is not critical for ActionCable to function properly; it's more of a "nice to have", so ActionCable is still fully functional even without that event handler getting invoked. With those two changes, ActionCable can run in a worker.

Other Information

To provide a working example, I've created the cable-in-web-worker branch in my fork of @palkan's anycable_demo repository. In that branch I extracted the ActionCable interaction to a shared worker using the changes from this PR.

[kaspth]

Is there anything Action Cable can do to schedule its own worker automatically? I don't think it's that uncommon to have one or more tabs open to a site, so just theoretically it sounds like there's gains here.

@jeremy @javan have you explored some of this at Basecamp?

[rmacklin]

@kaspth,

Yeah, it's possible in the future that the ActionCable library itself could offer built-in support for easily running inside a worker. It might wise to wait a bit for that, though. If we start small by proceeding with these changes, we'll enable users to explore moving cables into a worker themselves, and then we'd be in a better position to see the best patterns emerge and bring them into ActionCable.

[palkan]

IMO, it’s too opinionated to use Shared Worker by default (btw, AFAICS, it’s not supported by all major browsers yet).

Maybe, optional support for ActionCable.createSharedConsumer or smth like that (e.g. consumer adapetrization).
Or an ability to pass custom Socket wrapper.

[javan]

To avoid the document ReferenceError, I've added an if (typeof document !== "undefined") guard around the statements that add or remove the visibilitychange event listener.

That condition is pretty unclear out of context. Can it be extracted to an intention revealing helper?

Can the test suite be updated to simulate running in a worker scope so we don't accidentally reference window or document again in the future? What about these references?

rails/actioncable/app/javascript/action_cable/index.js

Lines 25 to 35 in a08827a

	export function getConfig(name) {
	const element = document.head.querySelector(`meta[name='action-cable-${name}']`)
	if (element) {
	return element.getAttribute("content")
	}
	}
	export function createWebSocketURL(url) {
	if (url && !/^wss?:/i.test(url)) {
	const a = document.createElement("a")
	a.href = url

[rmacklin]

IMO, it’s too opinionated to use Shared Worker by default (btw, AFAICS, it’s not supported by all major browsers yet).

Maybe, optional support for ActionCable.createSharedConsumer or smth like that (e.g. consumer adapetrization).
Or an ability to pass custom Socket wrapper.

Agreed, I wouldn't suggest we make that the default, just an option. And yeah, SharedWorker isn't supported everywhere, but fortunately it can fall back to a normal Worker which is widely supported:
https://github.com/rmacklin/anycable_demo/blob/3e31274f5526241a67531e7940e8214ecb7677c2/app/assets/javascripts/create-worker.js.erb#L1-L3

[javan]

Just curious: Would ActionCable run as-is in a web worker if you stubbed out window and document? Something like:

self.window = self.document = self;
require("actioncable");

[rmacklin]

What about these references?

Those references don't prevent the usage in a worker because you can pass url explicitly to createConsumer which prevents those functions from trying to reference document

[javan]

Right, but that's far from explicit, and as a reader of the code it's not clear why we can reference document freely in one place and have to test for it in another.

[rmacklin]

Can it be extracted to an intention revealing helper?

Are you thinking along the lines of:

const documentInterfaceAvailable = (typeof document !== "undefined")

or something else?

Can the test suite be updated to simulate running in a worker scope so we don't accidentally reference window or document again in the future?

Yeah, this is something I'd like to do. I'm not sure the best way to go about it, but my first idea was to try to fork https://github.com/Joris-van-der-Wel/karma-mocha-webworker and have it use QUnit instead of mocha, since that's what the existing test suite uses. Didn't want that to hold up this PR, though

[rmacklin]

Right, but that's far from explicit, and as a reader of the code it's not clear why we can reference document freely in one place and have to test for it in another.

Yeah. Initially I started making those two functions configurable like:

-export function createConsumer(url) {
+const defaultOptions = { getConfig, createWebSocketURL }
+
+export function createConsumer(url, options = defaultOptions) {
   if (url == null) {
-    const urlConfig = getConfig("url")
+    const urlConfig = (options.getConfig || defaultOptions.getConfig)("url")
     url = (urlConfig ? urlConfig : INTERNAL.default_mount_path)
   }
-  return new Consumer(createWebSocketURL(url))
+  return new Consumer((options.createWebSocketURL || defaultOptions.createWebSocketURL)(url))
 }

so that if you were using it in a web worker you could pass your own getConfig and createWebSocketURL that didn't rely on document. But after realizing those are both ignored if you explicitly pass a WebSocket url, I was thinking we could leave it and potentially just address this in the documentation.

What if the ActionCable guide called it out with something like:

If you are using ActionCable in a web worker, you'll need to pass the WebSocket url explicitly to createConsumer since the worker won't have access to the action-cable-url meta tag nor to document.createElement("a")

We could also add a comment above those methods warning that they don't work in a web worker.

[javan]

Can you give #34941 (comment) a try, please? I realize this is a small change, but it has long term maintenance implications, and I'd prefer to make no changes at all if there's a simple workaround.

[javan]

Are you thinking along the lines of const documentInterfaceAvailable = (typeof document !== "undefined") or something else?

Something else that reveals the intention. I'd prefer to read if (context.isWorker) { … } or if (context.isWindow) { … } or similar, where appropriate.

if (self instanceof Window) { … } could work too.

[rmacklin]

Can you give #34941 (comment) a try, please?

Sorry, I missed that comment before.

Just curious: Would ActionCable run as-is in a web worker if you stubbed out window and document? Something like:

self.window = self.document = self;
require("actioncable");

This works in isolation, but I would strongly recommend against it in a real application. Modifying global state like that to fake that the worker script is running in a window context will leak into all the other code in the worker. This is dangerous because it can break other library code used by the worker by tricking it into incorrectly assuming the worker is running in a window environment and using that assumption to access interfaces that aren't actually available.

I realize this is a small change, but it has long term maintenance implications, and I'd prefer to make no changes at all if there's a simple workaround.

I don't want to impose any long term maintenance commitment, so let's try to avoid that. How about we maintain that Rails doesn't officially support running ActionCable in a worker and that it could break at any time, while still avoiding the current ReferenceErrors with these changes. (It's actually been great how stable the ActionCable's javascript package has been since the release, so I wouldn't be surprised if it still worked in a year, but we can avoid making any support guarantees.)

Something else that reveals the intention. I'd prefer to read if (context.isWorker) { … } or if (context.isWindow) { … } or similar, where appropriate.

if (self instanceof Window) { … } could work too.

if (self instanceof Window) { … } is a lot nicer to read/more self-documenting, but unfortunately would also throw a ReferenceError since the Window constructor is similarly not defined in the worker global scope. But, I just pushed up a simpler alternative which is to call addEventListener and removeEventListener without the explicit receiver. That'll still work in the window context while effectively being a no-op in the worker context because the visibilitychange event won't ever get triggered there. Thus, we don't need to worry about abstracting the if (typeof document !== "undefined") checks because they've been removed.

[javan]

But, I just pushed up a simpler alternative which is to call addEventListener and removeEventListener without the explicit receiver.

Great! That's a perfectly reasonable compromise.

For some reason I thought the visibilitychange event didn't bubble, but I was wrong!

[javan]

Mind updating the PR description to reflect the final change? It still describes your original approach using if (typeof document !== "undefined"). Happy to merge after that.

[rmacklin]

Updated the PR description!

[rmacklin]

Thanks @javan! By the way, are you planning to attend RailsConf this year? Would love to buy you a drink for all your help 🍻

[javan]

Not 100% sure yet, but I hope to be there! 🍻