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

App crashes when passing in an empty Vec<u8> #165

Closed
ryanhossain9797 opened this issue Sep 21, 2023 · 11 comments
Closed

App crashes when passing in an empty Vec<u8> #165

ryanhossain9797 opened this issue Sep 21, 2023 · 11 comments

Comments

@ryanhossain9797
Copy link

ryanhossain9797 commented Sep 21, 2023

Report

I'm trying to make an app where I fire and forget a RustRequest, and rust returns DefaultResponse but also responds with a signal. I'm using that signal in a StreamBuilder to rebuild a widget, sort of like a state machine.

Seems to work fine in Web, but android apk seems to fail after a while. App will work fine for some seconds and then crash.

Also what's the best way to debug this?

Steps to Reproduce

Minimal example

  • Setup on freshly scaffolded flutter app with new Rust In Flutter template.
  • All default messages, sample functions, sample crate removed
  • Minimal example implemented in with_request.rs

action.proto

syntax = "proto3";
package action;

message AppAction {
  int32 screen_num = 1;
}

state.proto

syntax = "proto3";
package state;

message ScreenOne {}

message ScreenTwo {}

message AppState {
  oneof state {
    ScreenOne screen_one = 1;
    ScreenTwo screen_two = 2;
  }
}

main.dart

...

class _MyHomePageState extends State<MyHomePage> {
  @override
  Widget build(BuildContext context) {
    //Jumpstart the initial screen
    final rustRequest = RustRequest(
      resource: action.ID,
      operation: RustOperation.Read,
      message: action.AppAction(screenNum: 1).writeToBuffer(),
    );

    requestToRust(rustRequest);

    return StreamBuilder<RustSignal>(
        stream: rustBroadcaster.stream.where((rustSignal) {
      return rustSignal.resource == state.ID;
    }), builder: (context, snapshot) {
      if (!snapshot.hasData) {
        return const Center(
          child: CircularProgressIndicator(),
        );
      } else {
        var currentState = state.AppState.fromBuffer(snapshot.data!.message!);

        switch (currentState.whichState()) {

          //Screen Two

          case state.AppState_State.screenTwo:
            return Scaffold(
              body: const Center(child: Text("Screen Two")),
              floatingActionButton: FloatingActionButton(
                onPressed: () {
                  final rustRequest = RustRequest(
                    resource: action.ID,
                    operation: RustOperation.Read,
                    message: action.AppAction(screenNum: 1).writeToBuffer(),
                  );

                  requestToRust(rustRequest);
                },
                tooltip: 'Go To Screen One',
                child: const Icon(Icons.arrow_back_rounded),
              ),
            );

          //Screen One

          case state.AppState_State.screenOne:
          case state.AppState_State.notSet:
          default:
            return Scaffold(
              body: const Center(child: Text("Screen One")),
              floatingActionButton: FloatingActionButton(
                onPressed: () {
                  final rustRequest = RustRequest(
                    resource: action.ID,
                    operation: RustOperation.Read,
                    message: action.AppAction(screenNum: 2).writeToBuffer(),
                  );

                  requestToRust(rustRequest);
                },
                tooltip: 'Go To Screen Two',
                child: const Icon(Icons.arrow_forward_rounded),
              ),
            );
        }
      }
    });
  }
}

...

with_request.rs

...

    let rust_response = match rust_resource {
        // sample_functions::handle_counter_number(rust_request).await,
        messages::action::ID => {
            let message_bytes = rust_request.message.unwrap();
            let request_message =
                messages::action::AppAction::decode(message_bytes.as_slice()).unwrap();

            match request_message.screen_num {
                1 => {
                    let signal_message = AppState {
                        state: Some(ScreenOne(state::ScreenOne {  })),
                    };
                    let rust_signal = RustSignal {
                        resource: state::ID,
                        message: Some(signal_message.encode_to_vec()),
                        blob: None,
                    };
                    send_rust_signal(rust_signal);
                }
                2 => {
                    let signal_message = AppState {
                        state: Some(ScreenTwo(state::ScreenTwo {  })),
                    };
                    let rust_signal = RustSignal {
                        resource: state::ID,
                        message: Some(signal_message.encode_to_vec()),
                        blob: None,
                    };
                    send_rust_signal(rust_signal);
                }
                _ => {}
            }
            RustResponse::default()
        }
        _ => RustResponse::default(),
    };

...

System Information

❯ rustc --version
rustc 1.72.0 (5680fa18f 2023-08-23)

❯ protoc --version
libprotoc 24.2

❯ flutter doctor
Doctor summary (to see all details, run flutter doctor -v):
[√] Flutter (Channel main, 3.14.0-14.0.pre.332, on Microsoft Windows [Version 10.0.22621.2283], locale en-US)
[√] Windows Version (Installed version of Windows is version 10 or higher)
[√] Android toolchain - develop for Android devices (Android SDK version 34.0.0)
[√] Chrome - develop for the web
[√] Visual Studio - develop Windows apps (Visual Studio Community 2022 17.7.3)
[√] Android Studio (version 2022.3)
[√] VS Code (version 1.82.2)
[√] Connected device (3 available)
[√] Network resources

• No issues found!
@temeddix
Copy link
Member

Will inspect this soon, thanks for the report :)

@temeddix
Copy link
Member

temeddix commented Sep 22, 2023

This looks like an issue with zero-copy feature of allo-isolate, Dart VM handler written in Rust. This was why the app was working well on the web, which doesn't rely on allo-isolate. Removing that feature does solve this problem. I will make a PR that fixes this issue to that repository as soon as possible.

// native/hub/Cargo.toml
...
allo-isolate = { version = "0.1.19", features = [] }
...

For the time being, you can use this framework with that feature removed from allo-isolate. Thank you for your patience!

@temeddix
Copy link
Member

temeddix commented Sep 22, 2023

New version 4.2.1 published just now has fixed this bug, and I've also confirmed that it works well with your code. Try installing the new version and running rifs template again because the bridge folder inside hub has changed a little. Using zero-copy whenever possible would be more efficient on the memory.

I've also made a PR at allo-isolate that addresses this issue. However, RIF 4.2.1 solves this issue in our own way.

Thank you for telling us, and feel free to come back for any other discussions :)

@ryanhossain9797
Copy link
Author

ryanhossain9797 commented Sep 22, 2023

Hellow again @temeddix

The fix seems to work with the minimal example but still breaks in my real app. And I think I know why

            let response_message = messages::reset_action::ResetActionResult {};
            let empty_response = RustResponse {
                successful: true,
                message: Some(response_message.encode_to_vec()),
                blob: None,
            };

I noticed the fix you put in was to send a None instead of a Some(Vec::new()). In my case It seems since I'm sending an encoded struct that is empty, it's just an empty vec? (ResetActionResult is just an empty struct)

@temeddix
Copy link
Member

temeddix commented Sep 22, 2023

Yeah, you've got the right point. Right now, allo-isolate crashes when you put in an empty Vec<T>(With zero-copy enabled). This PR at allo-isolate that I've left solves the issue. Once that PR is merged, I will update the template to use the new version of allo-isolate right away. For now, you'll have to temporarily disable the zero-copy feature. 😅

@ryanhossain9797
Copy link
Author

ryanhossain9797 commented Sep 22, 2023

No worries. If allo-isolate gets fixed eventually workarounds are fine for now.

I'm actually just going to send a None there. This issue has made me realize I don't need the ResetActionResult type at all.

Thanks for the quick response

@temeddix
Copy link
Member

No problem :) On the other hand, I will keep this issue open until we have the new version of allo-isolate.

@temeddix temeddix reopened this Sep 22, 2023
@temeddix temeddix changed the title Android app crashes when using Signal with StreamBuilder App crashes when passing in an empty Vec<T> Sep 22, 2023
@temeddix temeddix changed the title App crashes when passing in an empty Vec<T> App crashes when passing in an empty Vec<u8> Sep 22, 2023
@temeddix
Copy link
Member

Now allo-isolate 0.1.20 rolled out and it completely fixes the zero-copy bug. I've confirmed that everything works as expected. It's also applied in RIF 4.4.0 template as well. 🎉

@ryanhossain9797
Copy link
Author

Awesome. Glad the issue got uncovered and fixed.

@LucaCoduriV
Copy link
Contributor

@temeddix
Just had the same issue.
I don't know if you changed that but the default value of RustResponse is an empty vec.

/// Response object that is sent from Rust to Dart.
#[derive(Clone)]
pub struct RustResponse {
    pub successful: bool,
    pub message: Option<Vec<u8>>,
    pub blob: Option<Vec<u8>>,
}

impl Default for RustResponse {
    /// Empty response with the successful value of false.
    fn default() -> RustResponse {
        RustResponse {
            successful: false,
            message: Some(Vec::new()),
            blob: Some(Vec::new()),
        }
    }
}

It's maybe better to have None ?

@ryanhossain9797
Copy link
Author

@temeddix
Just had the same issue.
I don't know if you changed that but the default value of RustResponse is an empty vec.

/// Response object that is sent from Rust to Dart.
#[derive(Clone)]
pub struct RustResponse {
    pub successful: bool,
    pub message: Option<Vec<u8>>,
    pub blob: Option<Vec<u8>>,
}

impl Default for RustResponse {
    /// Empty response with the successful value of false.
    fn default() -> RustResponse {
        RustResponse {
            successful: false,
            message: Some(Vec::new()),
            blob: Some(Vec::new()),
        }
    }
}

It's maybe better to have None ?

You're on an older version. It's been fixed already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants