Enable wasm-bindgen feature for the parking_lot dependency

[PhilippGackstatter]

I am building rust-libp2p as part of our Wasm bindings. The produced WebAssembly module ends up containing this import and call to a now function:

(import "env" "now" (func $now (type 3)))
...
(func $instant::wasm::now::h339f51d64be03d28 (type 3) (result f64)
  call $now)

This import then ends up in the wasm-bindgen-generated JavaScript file where the env import is not found and an error is produced.

According to this comment, this is the result of the dependency on parking_lot, where the fix is to enable the wasm-bindgen feature on that crate.

If the feature is enabled in rust-libp2p's Cargo.toml, then the import is no longer present and the issue is fixed.

I assume the best way to fix this, would be to enable the wasm-bindgen feature of the parking_lot crate when rust-libp2p's wasm-ext feature is enabled. However, I'm not sure if I'm not actually missing something, as this seems like an issue that should have come up in the past when compiling to wasm32-unknown-unknown.

Please let me know if you would like a PR that adds this or if there's another way to work around it.

[thomaseizinger]

I assume the best way to fix this, would be to enable the wasm-bindgen feature of the parking_lot crate when rust-libp2p's wasm-ext feature is enabled. However, I'm not sure if I'm not actually missing something, as this seems like an issue that should have come up in the past when compiling to wasm32-unknown-unknown.

Even better, we can dynamically enable the feature if we are compiling for wasm:

[target.'cfg(target_arch = "wasm32")'.dependencies]
parking_lot = { version = "0.11", features = ["wasm-bindgen"] }

What I don't quite understand is why this isn't directly done in parking-lot. Then things would just work out-of-the-box 🤷‍♂️

However, I'm not sure if I'm not actually missing something, as this seems like an issue that should have come up in the past when compiling to wasm32-unknown-unknown.

That is indeed strange. Maybe LTO is at work here and all the code has been optimized away because it wasn't used and your usecase is the first one where it is exposed?

[PhilippGackstatter]

Even better, we can dynamically enable the feature if we are compiling for wasm:

Right, good point! Although, the wasm-bindgen feature of parking-lot just re-exports the one from instant, and in their README they state that it's only relevant for wasm32-unknown-unknown (and asmjs, but I'm not sure how relevant that target still is). On wasm32-wasi the bindgen workaround isn't needed, and so the best thing to do may be to enable it only for the non-wasi Wasm target:

[target.wasm32-unknown-unknown.dependencies]
parking_lot = { version = "0.11", features = ["wasm-bindgen"] }

What I don't quite understand is why this isn't directly done in parking-lot

The wasm-bindgen feature assumes a JavaScript environment, but WebAssembly can run in standalone runtimes as well, where the JavaScript imports aren't available. Maybe that's why it's opt-in. Then again, not sure if the wasm32-unknown-unknown target is used much outside of JS environments, especially for libp2p. So opting-in for that target might be okay, what do you think?

[thomaseizinger]

just re-exports the one from instant

Might be worthwhile looking into porting our wasm-timer to that to reduce dependency bloat? But that is another story.

So opting-in for that target might be okay, what do you think?

Thanks for the detailed write-up!

I guess the cleanest way would be to leave the choice up to the user again but that is not very ergonomic.
I tempted to say we make it ergonomic for the majority of cases now and if someone comes along with a niche usecase, we can deal with it.

Ultimately, @mxinden will have to make a decision here.

[mxinden]

Thanks @PhilippGackstatter and @thomaseizinger for the discussion above.

However, I'm not sure if I'm not actually missing something, as this seems like an issue that should have come up in the past when compiling to wasm32-unknown-unknown.

Wild guess, Substrate - the main user of the libp2p in WASM - enabled the wasm-bindgen feature in parking-lot throughout its whole binary by importing parking-lot once more with the feature enabled. For example here:

https://github.com/paritytech/substrate/blob/c625c2ad924a5f58fbe4efbe937255c42eb3f7eb/bin/node/browser-testing/Cargo.toml#L25

Please let me know if you would like a PR that adds this or if there's another way to work around it.

Well, as far as I can tell, you could do something along the lines of the above, importing the same parking-lot version once more, setting the feature. I would consider this a big hack, thus, I would very much appreciate a PR with a proper fix.

Might be worthwhile looking into porting our wasm-timer to that to reduce dependency bloat? But that is another story.

Agreed @thomaseizinger. I think this is worth exploring.

I guess the cleanest way would be to leave the choice up to the user again but that is not very ergonomic.

Bubbling up the wasm-bindgen feature as initially suggested by @PhilippGackstatter would be my intuitive choice since I have come across this pattern before. That said I don't have much expertise in the Rust WASM space and thus might be the wrong person to make a decision.

Maybe @tomaka or @expenses has an opinion here?

[thomaseizinger]

Agreed @thomaseizinger. I think this is worth exploring.

Digging into this a bit, I found async-rs/futures-timer#51 which suggests that futures-timer would actually work properly on wasm32-unknown-unknown?
Together with https://github.com/sebcrozet/instant, we might not need wasm-timer at all any more?

It does look like the futures-timer needs some more work (f.e. async-rs/futures-timer#62). Additionally, it only works in browser-based wasm targets through its hard dependency on gloo-timers.

Bubbling up the wasm-bindgen feature as initially suggested by @PhilippGackstatter would be my intuitive choice since I have come across this pattern before. That said I don't have much expertise in the Rust WASM space and thus might be the wrong person to make a decision.

Even though it is not quite as ergonomic, I see now that it would be unnecessarily limiting to default to wasm-bindgen as it might prevent using rust-libp2p in non-browser WASM contexts.

I support the idea of bubbling up the wasm-bindgen feature.

[PhilippGackstatter]

enabled the wasm-bindgen feature in parking-lot throughout its whole binary by importing parking-lot once more with the feature enabled

I considered doing that, but because of other feature resolution issues we're now using resolver = "2", which, I think, breaks this hack - at least it doesn't work on our side. And generally, as soon as some crate in the dependency tree starts using the new resolver, this breaks down as well.

Together with https://github.com/sebcrozet/instant, we might not need wasm-timer at all any more?

I see now that it would be unnecessarily limiting to default to wasm-bindgen as it might prevent using rust-libp2p in non-browser WASM contexts.

Since opening this issue, I have also tried using rust-libp2p compiled to Wasm with node.js. One problem was that wasm-timer assumes a browser environment and fails when run in node.js, while instant can work around that with its inaccurate feature. So that change would be an improvement in that regard. Though, as you write, gloo-timers is also browser-only. I don't want to derail the conversation too much, but do you @thomaseizinger think it's feasible to get rust-libp2p running in node.js on wasm32-unknown-unknown? I would be prepared to do work here and perhaps you can help gauge how much that would. I've already come across a number of issues around timers and randomness APIs. I can also open a new issue if that's better.

Given that, I also agree with the feature based approach. However, wasm-bindgen works for both browser and nodejs, but needs to bind different APIs in that case. Most crates (e.g. parking_lot), it seems, simply assume a browser environment and thus wasm-bindgen always means browser. Depending on how feasible nodejs support is in the future, the feature here should perhaps be named accordingly, e.g. wasm-bindgen-browser or similar.

[thomaseizinger]

I don't want to derail the conversation too much, but do you @thomaseizinger think it's feasible to get rust-libp2p running in node.js on wasm32-unknown-unknown?

I think it should be possible but I don't know of libraries in the ecosystem that allow for this much flexibility. One option would be to build our own abstractions on top of instant. libp2p-core already depends on futures-timer. Instead of doing this, perhaps we can just depend on instant directly and build whatever we need ourselves, thus removing the dependencies on futures-timer and wasm-timer entirely.

To accelerate initial development and reduce friction, the Delay and Instant types could be re-exported from libp2p-core so they can easily be used by other libp2p crates. Once the interface stabilizes, we can think about moving them into a dedicated crate.

Given the complexity of the setup, I think it would be worthwhile to add dedicated tests like https://github.com/paritytech/substrate/tree/c625c2ad924a5f58fbe4efbe937255c42eb3f7eb/bin/node/browser-testing/src to our CI so we can:

a) Make sure these things work across various stacks, i.e. browser wasm, nodejs wasm, etc
b) Have minimal examples that show people what specifically needs to be done to make stuff work

I considered doing that, but because of other feature resolution issues we're now using resolver = "2", which, I think, breaks this hack - at least it doesn't work on our side.

I am surprised by this. It is my understanding that the primary feature of the new feature-resolver is that it will for example not activate a feature of a dependency that is specified only in a [target.wasm32-unknown-unknown.dependencies] block.

Given that, I also agree with the feature based approach. However, wasm-bindgen works for both browser and nodejs, but needs to bind different APIs in that case. Most crates (e.g. parking_lot), it seems, simply assume a browser environment and thus wasm-bindgen always means browser. Depending on how feasible nodejs support is in the future, the feature here should perhaps be named accordingly, e.g. wasm-bindgen-browser or similar.

That is not quite true I think.

Nodejs supports performance.now which is what instant uses by default with wasm-bindgen: https://github.com/sebcrozet/instant/blob/1f72ffddf0dbc4e6905d8543d113324d6967e038/src/wasm.rs#L111-L125

[PhilippGackstatter]

Sorry for the delay in response. And many thanks for spending the time on exploring this topic!

Nodejs supports performance.now which is what instant uses by default with wasm-bindgen: https://github.com/sebcrozet/instant/blob/1f72ffddf0dbc4e6905d8543d113324d6967e038/src/wasm.rs#L111-L125

Thanks a lot for pointing that out. This led me to try this in wasm-timer, and it seems to work. Something similar can be done for the setTimeout (see previous link) that wasm-timer uses, and I was able to compile rust-libp2p to wasm32-unknown-unknown and run it in node.js. Browser version also still works, which means that my suggestion for a separate wasm-bindgen-browser feature is moot.

Is that an acceptable fix for you, or do you think using instant and futures-timer is better?

With that "fixed", there is one other issue with a randomness source, but I have to investigate more and see if changing resolvers is a "fix".

For node.js, there are two more issue with the JS snippet that implements websocket transports. One is that a browser environment is assumed and listening is currently not implemented:

rust-libp2p/transports/wasm-ext/src/websockets.js

Line 25 in d931f36

let err = new Error("Listening on WebSockets is not possible from within a browser");

The other is how snippets work within wasm-bindgen. The transports/wasm-ext/src/websockets.js is an ES module, but node.js requires CommonJS modules. Yet wasm-bindgen doesn't rewrite those currently, so this is simply broken and needs to be fixed there. Just wanted to mention it for completeness. That can be worked around by manually editing the file, since the changes are relatively simple:

diff --git a/../../../rust-libp2p/transports/wasm-ext/src/websockets.js b/node/snippets/libp2p-wasm-ext-0d97e6de9e440efd/src/websockets.js
index 290af96..f7a9517 100644
--- a/../../../rust-libp2p/transports/wasm-ext/src/websockets.js
+++ b/node/snippets/libp2p-wasm-ext-0d97e6de9e440efd/src/websockets.js
@@ -18,7 +18,9 @@
 // FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER
 // DEALINGS IN THE SOFTWARE.
 
-export const websocket_transport = () => {
+const WebSocket = require('ws');
+
+const websocket_transport = () => {
        return {
                dial: dial,
                listen_on: (addr) => {
@@ -29,6 +31,8 @@ export const websocket_transport = () => {
        };
 }
 
+exports.websocket_transport = websocket_transport;
+
 /// Turns a string multiaddress into a WebSockets string URL.
 const multiaddr_to_ws = (addr) => {
        let parsed = addr.match(/^\/(ip4|ip6|dns4|dns6|dns)\/(.*?)\/tcp\/(.*?)\/(ws|wss|x-parity-ws\/(.*)|x-parity-wss\/(.*))(|\/p2p\/[a-zA-Z0-9]+)$/);

I am surprised by this. It is my understanding that the primary feature of the new feature-resolver is that it will for example not activate a feature of a dependency that is specified only in a [target.wasm32-unknown-unknown.dependencies] block.

Perhaps I have misunderstood how it works 🤔.

[PhilippGackstatter]

there is one other issue with a randomness source, but I have to investigate more and see if changing resolvers is a "fix".

To follow up on this, the issue was solved by specifying the getrandom dependency twice with the appropriate wasm-bindgen features, as we have dependencies in our tree that still depend on a 0.1 version.

getrandom = { version = "0.2", features = ["js"] }
getrandom01 = { package = "getrandom", version = "0.1", features = ["wasm-bindgen"] }

The other things are fixed by the two PRs I just opened (see references above).
After #2180 is merged, I would consider this issue fixed. The snippet topic is a whole different story and needs a fix in wasm-bindgen or we will find an ugly hack to auto-rewrite this in our node.js bindings build process, depending on the effort.

We might still open a PR to add listening support in the websocket.js snippet, unless there is disagreement there.

[thomaseizinger]

Is that an acceptable fix for you, or do you think using instant and futures-timer is better?

To be honest, I don't know. From what I can see, naming these features wasm_bindgen is actually wrong. They should be named performance-now and date-now or something like that. Plus I don't understand why instant even depends on wasm_bindgen and not just js_sys? It seems to only be accessing re-exported js_sys symbols.

For now #2180, seems to be the way forward as we basically have to follow upstream here in whatever they are doing.

[mxinden]

Following up on this issue and tomaka/wasm-timer#18, I would like to discuss once more, whether rust-libp2p should switch to https://github.com/sebcrozet/instant and https://github.com/async-rs/futures-timer. Not familiar with the technical barriers we might face, as a maintainer I am in favor of the move, given the larger community and the consolidation across the Rust WASM ecosystem.

@PhilippGackstatter @thomaseizinger @Thoralf-M do you think this is an option worth exploring?

[PhilippGackstatter]

Not familiar with the technical barriers we might face, as a maintainer I am in favor of the move, given the larger community and the consolidation across the Rust WASM ecosystem.

I'm not familiar with the technical barriers either, unfortunately. Given that instant already has node.js support, and the issue with the callback in wasm-timer, I would be in favor of that change, if it means a more stable Wasm version. @thomaseizinger already provided a rough idea, and it seems to be a feasible scope.

[thomaseizinger]

I am in favor of instant. Not sure about futures-timer because it depends on gloo-timers and I think that only works in browsers?

[wngr]

I'm currently trying to get the libp2p work within a web worker, where I bumped against the mentioned issue with wasm_timer. I guess removing the dependency on it and replacing it directly with instant and futures-timers (which libp2p already depends on) could be valid first step. With that, we would a) get rid of a (unmaintained?) crate, and b) improve support (web workers).

Not sure about supporting nodejs though.