Remove the app_control reply channel #183

swift-kim · 2021-09-09T09:08:28Z

Part of flutter-tizen/flutter-tizen#210.

Changes:

The reply channel and related code are completely removed. The sendLaunchRequest method now returns the response as a Map<String, dynamic> if waitForReply is true.
API changes: Change callerId to callerAppId and newly implement shouldReply.
Create a dedicated AppControlManager for managing AppControl instances. They were previously managed by AppControlChannel.
Minor changes. e.g. Change the type of AppControl IDs from int to int32_t, and use unique_ptr instead of shared_ptr.

Newly exported APIs:

The following new APIs were added to support automatic disposal of native app_control handles associated with Dart objects.
- NativeInitializeDartApi
- NativeCreateAppControl
- NativeAttachAppControl
The usage can be found here. Native handles created in this way do not need to be destroyed explicitly. The destruction callback is automatically called during GC. (The create and dispose methods are still supported just for backward compatibility.)
Refer to dart:ffi GC finalizers dart-lang/sdk#35770 for detailed information.
Please suggest better design and naming for these APIs!
NativeInitializeDartApi may be moved outside app_control.cc if more FFI functions are added in the future.

Notes:

Any existing code that uses app_control channels should still work after this change unless the code uses the reply channel.
The Dart wrapper of app_control channels is being worked in https://github.com/swift-kim/plugins/tree/add-tizen-app-control/packages/tizen_app_control. The package contains an example app so you can test this change with it.

I checked all native handles are properly disposed after GC (no memory leak) using the example app:

E/ConsoleMessage( 9853): app_control.cc: AppControl(49) > Create AppControl 1
E/ConsoleMessage( 9853): app_control.cc: AppControl(49) > Create AppControl 2
E/ConsoleMessage( 9853): app_control.cc: ~AppControl(75) > Destroy AppControl 1
E/ConsoleMessage( 9853): app_control.cc: ~AppControl(75) > Destroy AppControl 2
E/ConsoleMessage( 9853): app_control.cc: AppControl(49) > Create AppControl 3
E/ConsoleMessage( 9853): app_control.cc: AppControl(62) > Create AppControl 4
E/ConsoleMessage( 9853): app_control.cc: ~AppControl(75) > Destroy AppControl 3
E/ConsoleMessage( 9853): app_control.cc: ~AppControl(75) > Destroy AppControl 4
E/ConsoleMessage( 9853): app_control.cc: AppControl(49) > Create AppControl 5
E/ConsoleMessage( 9853): app_control.cc: AppControl(62) > Create AppControl 6
E/ConsoleMessage( 9853): app_control.cc: AppControl(49) > Create AppControl 7
E/ConsoleMessage( 9853): app_control.cc: AppControl(62) > Create AppControl 8
E/ConsoleMessage( 9853): app_control.cc: ~AppControl(75) > Destroy AppControl 7
E/ConsoleMessage( 9853): app_control.cc: ~AppControl(75) > Destroy AppControl 5
E/ConsoleMessage( 9853): app_control.cc: ~AppControl(75) > Destroy AppControl 6
E/ConsoleMessage( 9853): app_control.cc: ~AppControl(75) > Destroy AppControl 8
E/ConsoleMessage( 9853): app_control.cc: AppControl(49) > Create AppControl 9
E/ConsoleMessage( 9853): app_control.cc: AppControl(62) > Create AppControl 10
E/ConsoleMessage( 9853): app_control.cc: ~AppControl(75) > Destroy AppControl 10
E/ConsoleMessage( 9853): app_control.cc: ~AppControl(75) > Destroy AppControl 9

I'll do some more refactoring after this change. I tried not to make style changes other than logic changes in this PR.

- Let the method channel directly return responses to the caller - Add NativeInitializeDartApi, NativeCreateAppControl - Create AppControlManager

- Add NativeAttachAppControl - Move definitions to the header

bwikbs · 2021-09-13T07:30:21Z

shell/platform/tizen/channels/app_control.cc

+  }
+  auto id = app_control->id();
+  Dart_NewFinalizableHandle_DL(
+      handle, app_control.get(), 64,


Why is it 64? (Just curious.. 😄 )

The value is a hint for the garbage collector as far as the documentation says. Increasing the size may increase the chance of finalizable objects to be collected.

As a side note, the general finalizer support will be added to the Dart language in the near future so this (finalization through FFI) is a temporary solution until then.

WonyoungChoi · 2021-09-13T07:38:54Z

shell/platform/tizen/channels/app_control.cc

  }
  AppControlResult ret = app_control_reply_to_launch_request(
-      reply->Handle(), this->handle_, result_e);
+      reply->handle(), this->handle_, result_e);


Is there no need the null checking for AppControl*? other functions have same issues.

https://github.com/swift-kim/plugins/blob/ffe8d2536693eb1d222cb354b5b958813662f654/packages/tizen_app_control/lib/app_control.dart#L64-L93

An exception will be thrown in the Dart side constructors in case of instantiation failure so it cannot be nullptr as far as I can see (although it can be "invalid" if the destructor already has been called. The platform will return error if the app_control handle is invalid.)

Also I'll closely review the overall implementation in a follow-up PR. I tried not to make many changes to the existing code in this PR.

ok I understand.

WonyoungChoi · 2021-09-13T07:39:43Z

shell/platform/tizen/channels/app_control_channel.cc


 void AppControlChannel::Reply(
-    std::shared_ptr<AppControl> app_control,
+    AppControl* app_control,


bwikbs

I didn't see this and I've been looking at how it works for a long time. 😄
Results of the refactoring are expected !!(Very intuitive~!)
It's good try to use a finalizer for the part that is connected to dart in this way. 👍

WonyoungChoi

Great! clean code 👍

pkosko

Looks good to me.

* Remove the app_control reply channel - Let the method channel directly return responses to the caller - Add NativeInitializeDartApi, NativeCreateAppControl - Create AppControlManager * Convert shared_ptr to unique_ptr * Fix memory leaks - Add NativeAttachAppControl - Move definitions to the header

swift-kim force-pushed the app-control-rework branch from bda18b0 to 7c84c8a Compare September 10, 2021 01:56

swift-kim added 2 commits September 12, 2021 20:56

Remove the app_control reply channel

3543f93

- Let the method channel directly return responses to the caller - Add NativeInitializeDartApi, NativeCreateAppControl - Create AppControlManager

Convert shared_ptr to unique_ptr

cad4577

swift-kim force-pushed the app-control-rework branch from 49ba110 to 1f22bd7 Compare September 12, 2021 12:01

Fix memory leaks

f3011db

- Add NativeAttachAppControl - Move definitions to the header

swift-kim force-pushed the app-control-rework branch from 1f22bd7 to f3011db Compare September 13, 2021 01:19

swift-kim requested a review from a team September 13, 2021 04:16

bwikbs reviewed Sep 13, 2021

View reviewed changes

swift-kim mentioned this pull request Sep 13, 2021

[tizen_app_control] Add tizen_app_control package flutter-tizen/plugins#216

Merged

WonyoungChoi reviewed Sep 13, 2021

View reviewed changes

bwikbs approved these changes Sep 13, 2021

View reviewed changes

WonyoungChoi approved these changes Sep 13, 2021

View reviewed changes

This comment has been minimized.

Sign in to view

pkosko approved these changes Sep 13, 2021

View reviewed changes

swift-kim merged commit d9ef598 into flutter-tizen:flutter-2.2.1-tizen Sep 15, 2021

swift-kim mentioned this pull request Mar 8, 2022

Remove deprecated APIs in AppControlChannel #255

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove the app_control reply channel #183

Remove the app_control reply channel #183

Uh oh!

swift-kim commented Sep 9, 2021 •

edited

Loading

Uh oh!

bwikbs Sep 13, 2021

Uh oh!

swift-kim Sep 13, 2021

Uh oh!

WonyoungChoi Sep 13, 2021

Uh oh!

swift-kim Sep 13, 2021

Uh oh!

WonyoungChoi Sep 13, 2021

Uh oh!

WonyoungChoi Sep 13, 2021

Uh oh!

bwikbs left a comment

Uh oh!

WonyoungChoi left a comment

Uh oh!

This comment has been minimized.

pkosko left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Remove the app_control reply channel #183

Remove the app_control reply channel #183

Uh oh!

Conversation

swift-kim commented Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bwikbs Sep 13, 2021

Choose a reason for hiding this comment

Uh oh!

swift-kim Sep 13, 2021

Choose a reason for hiding this comment

Uh oh!

WonyoungChoi Sep 13, 2021

Choose a reason for hiding this comment

Uh oh!

swift-kim Sep 13, 2021

Choose a reason for hiding this comment

Uh oh!

WonyoungChoi Sep 13, 2021

Choose a reason for hiding this comment

Uh oh!

WonyoungChoi Sep 13, 2021

Choose a reason for hiding this comment

Uh oh!

bwikbs left a comment

Choose a reason for hiding this comment

Uh oh!

WonyoungChoi left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

pkosko left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

swift-kim commented Sep 9, 2021 •

edited

Loading