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

gloo-net: incorrect access of global when used in a worker #201

Open
cdata opened this issue Mar 24, 2022 · 8 comments
Open

gloo-net: incorrect access of global when used in a worker #201

cdata opened this issue Mar 24, 2022 · 8 comments
Labels
bug Something isn't working

Comments

@cdata
Copy link
Contributor

cdata commented Mar 24, 2022

Describe the Bug

gloo-net HTTP features cannot be used in a worker because the library always tries to access the Fetch API from Window, which will fail when running in a worker:

let promise = gloo_utils::window().fetch_with_request(&request);

Steps to Reproduce

  1. Design a WASM-compatible program that uses gloo-net to make HTTP requests
  2. Load and run it in a Web Worker

Expected Behavior

Expected HTTP requests are made.

Actual Behavior

An error, as the library attempts to access the wrong kind of global.

Additional Context

In digging around, I noticed there is some relevant history from gloo contributors: rustwasm/wasm-bindgen#2148 (comment).

Following that comment thread, I discovered that gloo previously incorporated a limited workaround for accessing global properties in a scope-agnostic way:

thread_local! {
static GLOBAL: WindowOrWorker = WindowOrWorker::new();
}
enum WindowOrWorker {
Window(Window),
Worker(WorkerGlobalScope),
}
impl WindowOrWorker {
fn new() -> Self {
#[wasm_bindgen]
extern "C" {
type Global;
#[wasm_bindgen(method, getter, js_name = Window)]
fn window(this: &Global) -> JsValue;
#[wasm_bindgen(method, getter, js_name = WorkerGlobalScope)]
fn worker(this: &Global) -> JsValue;
}
let global: Global = js_sys::global().unchecked_into();
if !global.window().is_undefined() {
Self::Window(global.unchecked_into())
} else if !global.worker().is_undefined() {
Self::Worker(global.unchecked_into())
} else {
panic!("Only supported in a browser or web worker");
}
}
}

In some cases, it may feasible to take a simpler approach, such as this patch I applied to another crate that has this same issue: devashishdxt/rexie@8637e4e

@cdata cdata added the bug Something isn't working label Mar 24, 2022
@cdata cdata changed the title gloo-net Incorrect access of global when used in a worker Mar 24, 2022
@cdata cdata changed the title Incorrect access of global when used in a worker gloo-net: incorrect access of global when used in a worker Mar 24, 2022
@cdata
Copy link
Contributor Author

cdata commented Mar 24, 2022

I have created a fairly ugly patch to make gloo-net usable in DedicatedWorkerGlobalScope contexts: master...cdata:master

@allsey87
Copy link

allsey87 commented Jun 8, 2022

I have created a fairly ugly patch to make gloo-net usable in DedicatedWorkerGlobalScope contexts: master...cdata:master

I wonder if a better solution would be to solve this at the wasm-bindgen level with a Global trait that is returned by js_sys::global and that covers all of the common APIs between Window and DedicatedWorkerGlobalScope. Note that there is also ServiceWorkerGlobalScope that should be considered in the design.

Alternatively we could have an enum returned from js_sys::global such as:

enum Global {
   Window(Window),
   DedicatedWorkerGlobalScope(DedicatedWorkerGlobalScope),
   ServiceWorkerGlobalScope(ServiceWorkerGlobalScope),
}

which could again cover all the common APIs between these types. Perhaps @alexcrichton has some thoughts on this?

@alexcrichton
Copy link
Contributor

Given how many contexts JS can run in I don't think that's a great idea for wasm-bindgen itself, but as a helper crate somewhere I think it definitely makes sense.

@cdata
Copy link
Contributor Author

cdata commented Jun 8, 2022

Thanks for the attention to this issue. Perhaps a gloo-global helper crate is in order?

@allsey87
Copy link

allsey87 commented Jun 8, 2022

My intuition would be to say that it belongs to gloo-util. @alexcrichton when you say that it's not a great idea for wasm-bindgen itself, are you also mean that it is not a good fit for js_sys?

@Zageron
Copy link

Zageron commented Aug 5, 2022

@cdata Thank you very much for your fork. I assume it's quite out of date now, but it still managed to resolve my issue.

@cdata
Copy link
Contributor Author

cdata commented Aug 5, 2022

@Zageron I don't have time to look at it right now, but if you happen to rebase the change I will happily accept a PR from you 🍻 cheers!

@flosse
Copy link
Contributor

flosse commented Sep 7, 2022

@cdata I rebased your commit and created a PR to hopefully have an upstream solution soon.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

5 participants