check core for wasm #228

ctaggart · 2021-04-11T17:40:16Z

For #37. This allows azure_core to target wasm. This builds off the http client work. It adds two features:

enable_request (default)
enable_hyper

ctaggart · 2021-04-11T17:45:16Z

An error I received was:

Rc<RefCell<wasm_bindgen_futures::Inner>>` cannot be sent between threads safely

Which led me to rustwasm/wasm-bindgen#2409 and then do the async_trait docs which say:

Not all async traits need futures that are dyn Future + Send. To avoid having
Send and Sync bounds placed on the async trait methods, invoke the async trait
macro as #[async_trait(?Send)] on both the trait and the impl blocks.

It worked for compiling azure_core but may cascade changes. I could use some help understanding the impact.

ctaggart

It compiles. 🥳 I'm sure I broke some stuff regarding Send. Conceptually, do we need the clients implementing Send? Please review in detail.

ctaggart · 2021-04-12T04:47:59Z

sdk/core/src/http_client.rs

@@ -92,7 +94,8 @@ impl HttpClient for hyper::Client<HttpsConnector<hyper::client::HttpConnector>>
    }
 }

-#[async_trait]
+#[cfg(feature = "enable_reqwest")]
+#[async_trait(?Send)]


This change appears to have a cascading effect. https://docs.rs/async-trait/0.1.48/async_trait/#non-threadsafe-futures

cfg_attr to the rescue! I've disabled Send from wasm builds only. Thanks to:
dtolnay/async-trait#159 (comment)

ctaggart · 2021-04-12T04:49:53Z

sdk/cosmos/src/clients/cosmos_client.rs

-        }
-    }
+    // TODO
+    // pub fn with_client(client: Arc<Box<dyn HttpClient>>) -> Self {


CosmosOption::with_client from #204 Policy Architecture does not compile now and I could use some help, @rylev.

Can you post the compiler error?

PS C:\Users\cataggar\io\azure-sdk-for-rust> cargo check Checking azure_cosmos v0.1.0 (C:\Users\cataggar\io\azure-sdk-for-rust\sdk\cosmos) error: future cannot be sent between threads safely --> sdk\cosmos\src\clients\cosmos_client.rs:51:17 | 51 | Box::pin(async move { Ok(client.execute_request(req).await?.into()) }) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ future created by async block is not `Send` | = help: the trait `std::marker::Send` is not implemented for `dyn futures::Future<Output = Result<http::Response<bytes::Bytes>, Box<dyn StdError + Sync + std::marker::Send>>>` note: future is not `Send` as it awaits another future which is not `Send` --> sdk\cosmos\src\clients\cosmos_client.rs:51:42 | 51 | Box::pin(async move { Ok(client.execute_request(req).await?.into()) }) | ^^^^^^^^^^^^^^^^^^^^^^^^^^^ await occurs here on type `Pin<Box<dyn futures::Future<Output = Result<http::Response<bytes::Bytes>, Box<dyn StdError + Sync + std::marker::Send>>>>>`, which is not `Send` = note: required for the cast to the object type `dyn futures::Future<Output = Result<azure_core::Response, Box<(dyn StdError + Sync + std::marker::Send + 'static)>>> + std::marker::Send`

ctaggart · 2021-04-12T04:50:43Z

sdk/event_grid/Cargo.toml

@@ -13,7 +13,7 @@ categories = ["api-bindings"]
 edition = "2018"

 [dependencies]
-azure_core = { path = "../core", version = "0.1.0" }
+azure_core = { path = "../core", version = "0.1.0", features = ["enable_hyper"] }


I'm hoping this is temporary. We can create an issue to make event_grid not require hyper.

sdk/storage/tests/blob.rs

.github/workflows/build.yml

sdk/core/src/http_client.rs

rylev · 2021-04-12T09:51:08Z

sdk/core/src/http_client.rs

@@ -122,6 +127,19 @@ impl HttpClient for reqwest::Client {
    }
 }

+// wasm can not get the http version


If I try to use it, I get:

PS C:\Users\cataggar\io\azure-sdk-for-rust> cargo check -p azure_core --target=wasm32-unknown-unknown Blocking waiting for file lock on build directory Checking azure_core v0.1.0 (C:\Users\cataggar\io\azure-sdk-for-rust\sdk\core) error[E0599]: no method named `version` found for reference `&reqwest::Response` in the current scope --> sdk\core\src\http_client.rs:140:19 | 140 | Some(response.version()) | ^^^^^^^ method not found in `&reqwest::Response`

The code shows this:
https://github.com/seanmonstar/reqwest/blob/1614c5ea64813ba24d268a2af22ab0d19b902bdf/src/wasm/response.rs#L67-L73

/* It might not be possible to detect this in JS? /// Get the HTTP `Version` of this `Response`. #[inline] pub fn version(&self) -> Version { self.http.version() } */

rylev · 2021-04-12T09:51:47Z

sdk/cosmos/src/clients/cosmos_client.rs

-        }
-    }
+    // TODO
+    // pub fn with_client(client: Arc<Box<dyn HttpClient>>) -> Self {


Can you post the compiler error?

sdk/storage/tests/blob.rs

.github/workflows/build.yml

rylev

Supporting wasm is definitely something we want. I think this needs some work though before we merge.

MindFlavor · 2021-04-12T11:38:47Z

I love this ❤️!

I am not clear why we can't have Sendable async traits. Is it a wasm requirement? That seems the only big blocker required for your PR (the pipeline code is being worked on now so I won't fixate too much on it).

ctaggart · 2021-04-14T16:57:54Z

sdk/core/src/http_client.rs

-#[async_trait]
+#[cfg(feature = "enable_reqwest")]
+#[cfg_attr(target_arch = "wasm32", async_trait(?Send))]
+#[cfg_attr(not(target_arch = "wasm32"), async_trait)]


I didn't know about cfg_attr, but thanks to dtolnay/async-trait#159 (comment), it works perfectly. Send is only disabled for wasm builds. Everything else works as before.

MindFlavor

Seems fine to me.
Nice work! 👍

build core for wasm

e21a589

ctaggart added 9 commits April 11, 2021 10:48

target: wasm32-unknown-unknown

07be89e

make get_version an option

8439d5d

set_redirect_uri to fix azure_identity

3cb59dd

comment out the CososOptions TODO block

4f47643

event_grid stuck on hyper for now

47c66e2

import all errors

2d0c6e7

comment CosmosClient::with_pipeline to compile

99eb6c0

enable_hyper for azure_service_bus

9e82c9b

remove requires_send_future in e2e test

2698c81

ctaggart commented Apr 12, 2021

View reviewed changes

rylev reviewed Apr 12, 2021

View reviewed changes

rylev suggested changes Apr 12, 2021

View reviewed changes

ctaggart mentioned this pull request Apr 12, 2021

Allow async traits rustwasm/wasm-bindgen#2409

Closed

check core instead of build

91463ca

ctaggart mentioned this pull request Apr 14, 2021

opt out of Send for wasm only dtolnay/async-trait#159

Closed

ctaggart added 3 commits April 14, 2021 09:43

cfg_attr to the rescue

4a56d83

remove spacing changes

69cac5e

remove change to fully qualified

0fe6cf1

ctaggart commented Apr 14, 2021

View reviewed changes

ctaggart changed the title ~~build core for wasm~~ check core for wasm Apr 14, 2021

ctaggart requested review from rylev and MindFlavor April 14, 2021 17:02

MindFlavor approved these changes Apr 16, 2021

View reviewed changes

rylev approved these changes Apr 19, 2021

View reviewed changes

ctaggart merged commit e1a0d62 into Azure:master Apr 19, 2021

ctaggart deleted the wasm_core branch April 19, 2021 15:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

check core for wasm #228

check core for wasm #228

ctaggart commented Apr 11, 2021 •

edited

Loading

ctaggart commented Apr 11, 2021 •

edited

Loading

ctaggart left a comment

ctaggart Apr 12, 2021

ctaggart Apr 14, 2021

ctaggart Apr 12, 2021

rylev Apr 12, 2021

ctaggart Apr 13, 2021

ctaggart Apr 12, 2021

rylev Apr 12, 2021

ctaggart Apr 13, 2021

rylev Apr 12, 2021

rylev left a comment

MindFlavor commented Apr 12, 2021

ctaggart Apr 14, 2021

MindFlavor left a comment

check core for wasm #228

check core for wasm #228

Conversation

ctaggart commented Apr 11, 2021 • edited Loading

ctaggart commented Apr 11, 2021 • edited Loading

ctaggart left a 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

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

rylev left a comment

Choose a reason for hiding this comment

MindFlavor commented Apr 12, 2021

Choose a reason for hiding this comment

MindFlavor left a comment

Choose a reason for hiding this comment

ctaggart commented Apr 11, 2021 •

edited

Loading

ctaggart commented Apr 11, 2021 •

edited

Loading