shekohex / allo-isolate

Run Multithreaded Rust along with Dart VM (in isolate) 🌀
Apache License 2.0
120 stars 18 forks source link

Feat/chrono #28

Closed Roms1383 closed 2 years ago

Roms1383 commented 2 years ago

This PR offers to add chrono types conversion in a feature-gated way. It would then allow supporting chrono types downstream in flutter_rust_bridge.

More details can be found in the original discussion, and in the subsequent related external PR.

These first commits are incomplete, hence the Draft status here : if approved, I'll then carry on with the missing changes.

As a short reminder, DateTime<Utc> and sibling types (NaiveDateTime and the likes) all boils down to i64 under the hood. Notably though, Dart handles precision and representation differently in the VM (microseconds based) and in JavaScript (milliseconds based). As far as I understand, the nitty-gritty details would actually be the responsibility of downstream crate since for example flutter_rust_bridge will handle generating code wiring it through FFI to be able to reconstruct a DateTime on Dart side, a DateTime<Utc> on Rust side , and whichever appropriate counterpart on JavaScript side (haven't looked into it yet but shouldn't be too much of a concern).

update: my bad, there's no need to care about JS/WASM as rightfully pointed out.

Roms1383 commented 2 years ago

the nitty-gritty details would actually be the responsibility of downstream crate (...)

Well maybe I should be clearer on this point : probably allo-isolate responsibility is to make sure to turn e.g. DateTime<Utc> into a i64 using timestamp_micros() in most targets, while if aiming at web target with timestamp_millis().

But (as far as I understood), the remaining conversions (to Dart DateTime from Rust, to Rust DateTime<Utc> from Dart, so on and so forth...) is handled by downstream crate, in my case flutter_rust_bridge.


And, if ever there's a safety concern that I can think of, it's when the data is hydrated from FFI, like:

// taken from potentially generated code in flutter_rust_bridge at frb_example/with_flutter/rust/src/api.rs
impl Wire2Api<chrono::DateTime<chrono::Utc>> for i64 {
    fn wire2api(self) -> chrono::DateTime<chrono::Utc> {
        chrono::DateTime::<chrono::Utc>::from_utc(
            chrono::NaiveDateTime::from_timestamp(self, 0), // <--- here indeed 'from_timestamp' can utterly panic, e.g. in case of overflow
            chrono::Utc,
        )
    }
}

But since usually Dart DateTime / Rust DateTime<Utc> will all be created rightfully (after all if you own a DateTime<Utc>, it is guaranteed to be valid, as far as I'm aware), there's no reason it ever happens.


Let me know if there's more concern that I missed, or if you want to further discuss about it. Thanks :)

shekohex commented 2 years ago

try cargo +nightly fmt for formatting and linting.

Roms1383 commented 2 years ago

@shekohex how would you add a test to check that the value sent on one side (e.g. Rust) is the same value received on the other side (e.g. Dart) with allo-isolate ? Also I'm still checking about DateTime<FixedOffset> for the best way to convert it (and I have a couple of pending commits coming too)

Roms1383 commented 2 years ago

So in the meantime I'm comparing conversions on both playgrounds (Rust playground and Dart pad) : Rust playground / Dart pad sample

Roms1383 commented 2 years ago

update: I made a mistake and was corrected in the Dart SDK related issue thread. On platform other than web, there's no lossy conversion.

So it seems that Dart DateTime has a minor bug when converting from microseconds: a quick demo in dartpad to illustrate, and there's also a related opened issue.

Roms1383 commented 2 years ago

update: I made a mistake and was corrected in the Dart SDK related issue thread. On platform other than web, there's no lossy conversion.

So microseconds get truncated during conversion, tested with:

But since this issue will be fixed sooner or later in Dart SDK, we can probably ignore it. In the meantime, people who would really need microseconds precision probably already have come up with a custom solution on their own (e.g. creating a class containing both a DateTime and a separate int which keep track of the microseconds).


Aside from that, the more I think of DateTime<FixedOffset>, the more I think it's ok not to implement it since DateTime only expose a getter to retrieve the difference between utc and local time, namely timeZoneOffset, and all its constructors are local time based or utc based.

shekohex commented 2 years ago

how would you add a test to check that the value sent on one side (e.g. Rust) is the same value received on the other side (e.g. Dart) with allo-isolate

Currently, there are no dart tests in this repo, we mocked the dart vm behavieur to test things out without the need for dart setup in the CI or locally.

Roms1383 commented 2 years ago

Currently, there are no dart tests in this repo, we mocked the dart vm behavieur to test things out without the need for dart setup in the CI or locally.

Indeed, I'll add them on flutter_rust_bridge.

Roms1383 commented 2 years ago

@shekohex waiting for your review :)

shekohex commented 2 years ago

Published in v0.1.14-beta.2