Viaduct replacement demo #6018

bendk · 2023-12-21T19:27:57Z

This shows off a potential viaduct replacement that uses new UniFFI features.

The 1000ft view is that:

We define an HTTP backend trait in Rust (note that the method is async, via the async_trait crate)
We can implement that trait in Rust
or implement it in Python
We initialize the crate with one of those backends
Then we can make HTTP requests in Rust, independent of the implementation.

Check out the fairy-bridge README and the demo README for details. Execute run-demo.py to test it out yourself.

The UniFFI features are still a WIP. This is currently using a branch in my repo. The current plan for getting these into UniFFI main is:

Get the 0.26.0 release out the door
Merge PR Move 'devices' to the top-level of the sync ping #1818 into main
Merge my async-trait-interfaces branch into main (probably using a few smaller PRs)

The Desktop plan needs to be explored more. I believe there should be a way to use Necko in Rust code, but that needs to be verified.

I made this using UniFFI proc-macros, to test out the technology and I have to say it was very pleasant to work with.

codecov-commenter · 2023-12-21T19:43:46Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (main@a8c15ba). Learn more about missing BASE report.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #6018   +/-   ##
=======================================
  Coverage        ?   22.64%           
=======================================
  Files           ?      330           
  Lines           ?    29814           
  Branches        ?        0           
=======================================
  Hits            ?     6750           
  Misses          ?    23064           
  Partials        ?        0

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

mhammond

Far out, this is fantastic - it's small and simple. I guess seeing what the necko backend looks like is where the fun starts? Either way, I think this is worth pursuing.

mhammond · 2024-01-03T16:33:51Z

components/fairy-bridge/README.md

+
+## Cookies / State
+
+Cookies are explicitly not supported at the moment, adding them would require a separate security


I know you copy/pasted this, but can we tighten this more? eg, "Cookies and state are outside the scope of this library. Any such functionality is the responsibility of the consumer"

mhammond · 2024-01-03T16:34:06Z

components/fairy-bridge/README.md

+
+## Name
+
+`fairy-bridge` is named after the Fairy Bridge (Xian Ren Qiao) -- the largest known natural bridge in the world, located in northwestern Guangxi Province, China.


mhammond · 2024-01-03T16:36:57Z

components/fairy-bridge/src/backend.rs

+pub struct BackendSettings {
+    // Connection timeout (in ms)
+    #[uniffi(default = None)]
+    pub connect_timeout: Option<u32>,


Again, I'm sure you copied these, but I wonder if these are actually appropriate defaults for timeouts? If someone neglects to think about this, it seems better the request times out too soon than never times out? Or maybe make them a non-option - I'm not sure it ever makes sense to say "it's fine if this request literally never completes" - much like we are implicitly also not allowing "it's fine if redirects never complete" below.

Great point, but we'll need a UniFFI fix for this: mozilla/uniffi-rs#1925

While that PR makes sense, do we really need that here? ie, can we just make those values u32? I think that would offer ~600 hours of timeout?

(I don't really care, it's more just random thoughts about the API rather than a concrete suggestion)

mhammond · 2024-01-03T16:45:58Z

components/fairy-bridge/src/lib.rs

+mod error;
+pub mod headers;
+mod request;
+#[cfg(feature = "reqwest")]


there doesn't appear to be a feature of this name? (I was going to ask whether it should be, say, "backend-reqwest" or similar, even though it really doesn't matter :)

The feature is implicit because of the optional dependency. But "backend-reqwest" sounds better to me too.

mhammond · 2024-01-03T16:47:39Z

components/fairy-bridge/src/lib.rs

+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+use once_cell::sync::OnceCell;


should be now be using std::cell::OnceCell?

mhammond · 2024-01-03T16:49:26Z

components/fairy-bridge/src/request.rs

+#[derive(uniffi::Record)]
+pub struct Request {
+    pub method: Method,
+    pub url: String,


many of your fns use Url - should we try and use that here and leverage custom types?

This makes a lot of sense, but I'm slightly worried about conversion errors. If Request.url is an Url, then Response.url maybe also should be. But that means that a request could fail if the foreign code returns an invalid URL. Maybe that's bad? I'm not really sure about this one.

mhammond · 2024-01-03T16:52:10Z

components/fairy-bridge/src/request.rs

+    /// this to fail hard with an easy to track down panic, than for e.g. `sync`
+    /// to fail with a JSON parse error (which we'd probably attribute to
+    /// corrupt data on the server, or something).
+    pub fn json<T: ?Sized + serde::Serialize>(mut self, val: &T) -> Self {


the trait bounds are cool, but I wonder if, for all the reasons you mention in the comment, this should just take serde_json::Value?

I tried that out, but I felt like it made the consumer code kind of awkward. I do find this docstring a bit weird though. I'm going to update the trait bounds a bit and return a Result. I think it's more natural for consumers to deal with serialization errors than have the function panic.

mhammond · 2024-01-03T16:54:01Z

components/fairy-bridge/src/request.rs

+    pub fn json<T: ?Sized + serde::Serialize>(mut self, val: &T) -> Self {
+        self.body =
+            Some(serde_json::to_vec(val).expect("Rust component bug: serde_json::to_vec failure"));
+        if !self.headers.contains_key(headers::CONTENT_TYPE) {


it would seem odd if the existing header had anything but app/json? (Not sure what you should do in that case - if it turns out a use-case for this was legitimate, even logging would be a PITA. OTOH, if there were no valid use-cases, then this would probably result in hard-to-track-down errors which naively might also be attributed to the server instead of the app

Agreed. Let's just force the header to be application/json. json() is just a convenience method after all, the user can always serialize the data themselves.

mhammond · 2024-01-03T16:55:35Z

components/fairy-bridge/src/reqwest_backend.rs

+pub fn init_reqwest_backend(settings: BackendSettings) -> Result<(), FairyBridgeError> {
+    // Create a multi-threaded runtime, with 1 worker thread.
+    //
+    // The difference between this and the current thread worker, is that it will manage the worker


I don't understand what "the current thread worker" is in this context - the current thread is likely to just be a normal thread and not associated with tokio at all, right? (well, tbh I don't really understand this comment at all :)

It's a tokio thing. The runtime always piggybacks on the current thread. You can spawn tasks on the worker, but nothing happens until you call block_on, which then blocks the current thread while the tasks execute. I thought it would be the right runtime, since we only need one thread, but the execution model is very different. I'll try to update this comment to make it more clear.

mhammond · 2024-01-03T17:00:51Z

components/fairy-bridge/src/response.rs

+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+use std::collections::HashMap;


seems like it might be fine for this to be in lib.rs? (Also fine not to be, I'm just throwing comments out there as they fleetingly enter and leave my brain :)

bendk · 2024-01-03T21:01:46Z

Thanks for the review! I just updated a bunch of code based on the suggestions. I think you're right that the real interesting part will come from integrating with Necko. If the UniFFI bits are the boring part, I guess that's a good sign.

This shows off a potential viaduct replacement that uses new UniFFI features. Check out `components/fairy-bridge/README.md` and `examples/fairy-bridge-demo/README.md` for details. Execute `examples/fairy-bridge-demo/run-demo.py` to test it out yourself. The UniFFI features are still a WIP. This is currently using a branch in my repo. The current plan for getting these into UniFFI main is: - Get the `0.26.0` release out the door - Merge PR mozilla#1818 into `main` - Merge my `async-trait-interfaces` branch into main (probably using a few smaller PRs) There is a C++ backend using libcurl. I think that necko shouldn't be any harder than this.

bendk added WIP Work in progress discuss labels Dec 21, 2023

bendk changed the title ~~Viadict replacement demo~~ Viaduct replacement demo Dec 21, 2023

bendk force-pushed the fairy-bridge branch 2 times, most recently from 88045a9 to 92d1861 Compare December 21, 2023 19:33

bendk force-pushed the fairy-bridge branch 2 times, most recently from 0810d75 to 11a0ba5 Compare December 21, 2023 19:52

mhammond reviewed Jan 3, 2024

View reviewed changes

bendk force-pushed the fairy-bridge branch from 11a0ba5 to e83357a Compare January 3, 2024 21:00

bendk force-pushed the fairy-bridge branch 8 times, most recently from 8272beb to dac259b Compare January 11, 2024 20:06

bendk mentioned this pull request May 3, 2024

New system for remote / custom / external types mozilla/uniffi-rs#2087

Closed

bendk force-pushed the fairy-bridge branch 2 times, most recently from 476e13f to c003902 Compare May 21, 2024 16:40

bendk force-pushed the fairy-bridge branch from c003902 to 5f60e78 Compare June 21, 2024 14:24

bendk mentioned this pull request Jun 21, 2024

Switch to sync code (again) mozilla-services/remote-settings-client#418

Closed

bendk force-pushed the fairy-bridge branch 2 times, most recently from 51e7bb6 to d1d6654 Compare June 21, 2024 15:45

bendk force-pushed the fairy-bridge branch from d1d6654 to faa4d4a Compare June 21, 2024 16:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Viaduct replacement demo #6018

Viaduct replacement demo #6018

bendk commented Dec 21, 2023 •

edited

Loading

codecov-commenter commented Dec 21, 2023 •

edited

Loading

mhammond left a comment

mhammond Jan 3, 2024

mhammond Jan 3, 2024

mhammond Jan 3, 2024

bendk Jan 3, 2024

mhammond Jan 3, 2024

mhammond Jan 3, 2024

bendk Jan 3, 2024

mhammond Jan 3, 2024

mhammond Jan 3, 2024

bendk Jan 3, 2024

mhammond Jan 3, 2024

bendk Jan 3, 2024

mhammond Jan 3, 2024

bendk Jan 3, 2024

mhammond Jan 3, 2024

bendk Jan 3, 2024

mhammond Jan 3, 2024

bendk commented Jan 3, 2024


		## Cookies / State

		Cookies are explicitly not supported at the moment, adding them would require a separate security


		## Name

		`fairy-bridge` is named after the Fairy Bridge (Xian Ren Qiao) -- the largest known natural bridge in the world, located in northwestern Guangxi Province, China.

Viaduct replacement demo #6018

Are you sure you want to change the base?

Viaduct replacement demo #6018

Conversation

bendk commented Dec 21, 2023 • edited Loading

codecov-commenter commented Dec 21, 2023 • edited Loading

Codecov Report

mhammond 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

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

bendk commented Jan 3, 2024

bendk commented Dec 21, 2023 •

edited

Loading

codecov-commenter commented Dec 21, 2023 •

edited

Loading