Add support for blocking platform message send #4358
Conversation
mravn-google
requested review from
sigurdm and
jakobr-google
and removed request for
sigurdm
| * bytes between position zero and current position, or null. | ||
| * @return a {@link ByteBuffer} containing the reply payload, or null. | ||
| */ | ||
| ByteBuffer sendSync(String channel, ByteBuffer message); |
There was a problem hiding this comment.
Perhaps rename to sendBlocking?
|
We already have a flutter/flutter test of channel communication. I'll add some roundtrip cases to that once the engine PR lands and rolls. |
mravn-google
changed the title
|
Are you sure this feature doesn't introduce a deadlock? Which threads are allowed to block on which other threads? |
|
@abarth The platform thread is allowed to block waiting for the ui thread. |
|
@mravn-google That's good to know. Is the blocking pattern between the threads documented somewhere? It's the kind of documentation I'd find useful to refer to in the future. |
|
@abarth It is explicitly documented (in the two platform-specific So: the wait is ended when Dart code responds to the platform message. What is the official term for the thread executing in the Dart isolate? I called it the ui thread above. |
|
I'd call it the "thread on which the framework handles the message" or some such. |
|
I'm very dubious about doing anything blocking. The way we've solved this problem in the past (e.g. in the many accessibility APIs that rely on synchronous response) is to precompute the data and have it around when it's necessary. Why can't we use that approach here? |
|
@Hixie In the particular case driving this PR, that would require the Dart code to keep available a serialized form of whatever view state, the app developer cares about saving and restoring. I figured that might be expensive. |
I guess I would expect the find the documentation in this class, where the threads are declared: https://github.com/flutter/engine/blob/master/common/threads.h#L30 The import issue is the global pattern of which threads are allowed to block on which other threads, among all the features of the engine. If there's a cycle in that pattern, then there's the possibility of a deadlock. The trouble with documenting the blocking in the platform-specific |
Don't you need to do that anyway to survive a crash without losing state? It shouldn't be that expensive, surely. A few milliseconds whenever the view state changes, which is pretty rarely (when the user changes route, when they scroll maybe...). I think it would be significantly preferable to introducing blocking APIs. The problem with blocking APIs is that they're initially used only for those really hard cases where you have "no choice", and then they're used when you have a choice but it's "really hard" without blocking, then they're used because "well we use it over there", and before you know it, everything is blocking and you might as well only be using one core. Not to mention the imminent deadlock issues when someone starts waiting on something else and we eventually loop it all around, like @abarth is worrying about. |
|
To offer additional perspective: hybrid HTML5 apps on Android have this same problem, since (as far as I know) there's no way to send a synchronous message from native to HTML5. (iOS has a synchronous API, but they discourage its use). The way we dealt with this on past projects was to save the state at each point where it changed significantly. In our case this was a local save, although a cloud save would be similar. For performance reasons we batched state changes until execution reached the top of stack, then did the persistence. Since the persistence was asynchronous it didn't block UI. One advantage of this approach over responding to lifecycle events is that it saves state in cases where the lifecycle events don't happen, which is rare but possible (e.g. sudden battery disconnect). |
|
I strongly prefer that we not introduce blocking APIs in the engine for the following reasons:
The code looks good but I am blocking the PR for these reasons. If the rest of the team decides otherwise, feel free to override. |
|
@mravn-google @Hixie @abarth @chinmaygarde I think that the importance of having synchronous methods has been underestimated: they are (at least on Android) very much necessary! I'm saying this not just because serializing the instance state at every change sounds far from an ideal solution, but because that's just one of literally a myriad of cases where the host system expects a synchronous response, as a contract, and the trick of pre-computing the response is almost never a possible solution. At the cost of stating the obvious, it's worth mentioning that Android apps currently do block the thread when performing these operations, so any code ported to Flutter would also transfer the concern of not blocking more than necessary. Keep in mind that this is something that is only going to be used in platform plugins and can potentially be enabled only on platforms that really need it, like Android. We want Flutter to be able to fully integrate with the system reaching 100% feature parity, and I strongly believe this is one of the steps that needs to be taken. |
|
This PR was a solution to a particular use case (state persistence). Perhaps it would be worth filing issues for the other use cases you have in mind? |
|
@krisgiesing @Takhion Actually, this PR attempted to provide a general solution to involving Dart code execution in synchronous platform call contexts. But I agree it would be good to list a few other scenarios where having Dart code keep the (serialized) result up to date could be problematic. |
|
@Takhion: It is entirely possible that we will have to support synchronous platform calls. My objections to this patch are mainly centered around construction issues (deadlocks in case of task runner threads being the same, hangs with ANRs in case of dropped messages, performance guarantees of message responses, etc..).
There is a distinction between performing operations on the current thread vs. waiting for another thread to perform tasks while the current thread idles on a countdown latch. @mravn-google: I wonder if it is possible to support synchronous execution of the callback by running a Dart invocation on the platform thread itself. We would have to make sure all dart invocations are made after acquiring a binary semaphore. But it should be theoretically possible (and safer). We can make the runtime controller hold the semaphore. WDYT? @Takhion: I want to re-iterate that I am not against supporting synchronous platform calls. But, we have to figure out a scheme that is safe against deadlocks and hangs. And, even if we do implement such a scheme, there will still be a performance overhead related to making such calls. So, I would like for us to resort to such schemes as a means of last resort. |
|
FWIW, I am against us making it easy to perform synchronous calls from the Java thread to the dart:ui isolate, unless there's a really strong use case. I haven't seen a use case yet that is compelling enough to require that we support this. |
|
@Takhion would it be too much trouble to help us see a list of the many sync APIs that might require interop'ing with? You are very experienced with Android, it would be a big help. |
|
@sethladd of course! Sorry if I'm available only intermittently but I can only work on this in my (little) spare time and we just moved to a new flat and still don't have an Internet connection at home... BUT I'm writing a proper reply with use-cases, stay tuned 💪 |
|
@sethladd @Hixie -- Not sure if @Takhion got sidetracked, but to show some progress on this, I'll help fill in some of them that exist... And why we need some sort of Sync support... Some SYNC functions:
These are all on Activity, Application or Fragments callbacks. That doesn't even count any normal Listeners, which are key in Android for a huge number of events and a lot of them are syncronise. Please note this is NOT a complete list; just some of them I found by looking quickly in the ASOP source. Some of these EXPECT a returned value which changes the rest of what the calling function will do when called. Some of these if they are ASYNC the code after the call (which expected you to do something) will then continue and so you never had time to do what you needed. For example; onStop, onDestroy, onPause, onLowMemory, onTrimMemory, onSaveInstanceState all run code AFTER they call you and so they expect you to have handled whatever they sent you first; and then they proceed to do the next step in their process which can be a bad thing if you weren't able to handle it. Some of these above I realize are moot in Flutter because they aren't ever triggered unless we are attempting to overlay native view over the flutter window... The ones like |
|
Some use cases:
I've also written about 30 cross-platform plugins for a competing framework (NativeScript) and I can tell you there is a whole lot of Synchronized callbacks on both iOS and Android. So if I can't call into Dart from the Android or iOS side and easily wait for the Dart side callback to generate and return a value that I then return to the native framework I'm calling. These plugins could NEVER be done in Flutter (one of them that I worked on last week is Google's GVRKit, it uses a Sync callback on iOS to return a created and customized control) So I think we DO need a standard way to do Sync callbacks for at least plugin developers to be able to easily bring NATIVE stuff into Flutter properly in the places where the Framework expects and requires it... |
|
I hear what you're saying. Sync callbacks are simply a non-starter though. They make it WAY too easy to deadlock your application, let alone worries like introducing jank and so on. In general we can implement most of these by having the Dart side proactively tell the OEM side what to reply when a request comes in. For some others, we'll have to do more of the work on the OEM side. |
|
@Hixie - Ian, I'm not sure you really do get what I'm saying if you aren't willing to do anything Sync, and you just dismiss it because it is not something you like. I do realize it makes it a bit/lot more "messy" and isn't perfect, but the platforms you are supporting aren't perfect and you have NO control over them. Sometimes to make your stuff work inside the ecosystem properly you have to unfortunately get a little messy. Let me give a bit of context, so maybe my opinion will have a bit more sway, since I'm new to this community and you don't know me from a $5 dollar developer from the north pole. 😀 I've written a couple books on NativeScript, I've produced ~30 cross platform plugins for NativeScript everything from driving portable bluetooth printers , Sqlite, ExoPlayer, SciCharts, Wikitude, & GRVKit integration. I've dug deep into the v8 and JavaScript core engines, and literally can build & debug the entire NativeScript stack. I've written both pure native and NativeScript apps on both ios and android platforms. So I am very well versed in the mobile platforms, including the requirements to make apps that work properly for the end users, which is the most important piece of the pie. (Oh, and I also do Desktop & DevOps Server work, be hard to find a field I haven't done well in. 😀 ). Hopefully this gives you a general idea of my background and knowledge base. Now onto this actual issue... :D I'll give you a couple REAL examples. Tonight I was working on some Swift and ObjC code -- Google's own GVRKit requires that at certain points it will pass me a value and I must respond with a different type of newly instantiated memory HEAVY classes each time. They free the prior classes. I CANNOT do this AOT, these classes will conflict with each other and each have some serious resources associated with them. (i.e. they are basically the entire render stack for which ever 3d mode that the user is wanting to activate at that moment). Their is no way to pre-decide and pre-build this -- this is 100% completely dependent on what the OS side passes in from the users choices. And it will completely change what entire components tree I return. And this is 100% sync, where I have to RETURN the class (& its tree) at that moment. Without sync support, GVRKit is pretty much unsupportable in Flutter since it is NOT open source! Also things like (android) OnLowMemory, (android) onTrimMemory, (ios) applicationDidReceiveMemoryWarning - it is CRITICAL that you handle this in a Synchronous manner, because when it returns back to the OS side; the OS side then runs GC and then re-evaluates right then if it needs to take a more drastic actions and just kill the app. So, if you handicap flutter and just send a ASYNC message to the dart side to clean memory and return right away to the OS -- bye bye flutter app. Where a normal app can easily clear some heavy resources, and then return. Which will typically leave a much nicer experience for the user... Again, the whole reason the OS is asking you about releasing resources, is because your app is hogging them. It is giving you an opportunity to be friendly before it kills you, and you just choose to handicapped Flutter from being a good citizen on BOTH platforms. In addition to those two use cases; if you do manage to integrate Native Views into the Flutter rendering system (which would help grow the ability for us to do a lot of visual plugins); You need to realize on iOS the majority of the "native" controls have delegates and they are pretty much are all synchronous calls. Android can has a similar issue with some controls; but most of them are NOT async when dealing with controls, so it is less of an issue. Yes some of it can be pre-computed; but a lot of it cannot be pre-decided and you need to run code somewhere (hopefully in Dart) to decide which answer to give the OS. And a final example; several months ago I worked with a client on a cross platform NativeScript app. One feature he wanted was to use the Up/Down volume key buttons to trigger different actions depending on where he was in the app. So in this case, I had to determine what field and screen I was on to decide if I needed to handle the request OR if I wanted to let the OS handle it as actual volume control event. The interface was very natural for the end users, so it made sense to them. The ONLY way to capture this information on Android (& ios) is the application wide In NativeScript, the plugin fires my event code -- I grab the value from the framework about which screen I'm on and then ask the framework what field I'm on. If no field is selected I can return out of my routine and just let the OS handle the event properly (i.e. return false). If I'm on a field, I can look at the field type and/or my simple array of field names that the up/down applies to and see if the names match and then return true/false to the OS. Again, a simple array of field names is all I need for the special cases. Maintenance is pretty minimal and if I add any more special fields to any screen, I just need to add it to once to the array. This solution is totally cross platform, and I only maintain one small set of code base in the app to handle everything for the entire application. Pre-calculating on each change of the field is IMHO a total nightmare for Flutter... Finally, I also just took a few minutes to look at ARKit, ARCore, Google Maps; and ExoPlayer. Three out of the Four of those cannot ever be FULLY supported in Flutter without some sort of synchronous support. I only briefly looked at them, so this is after glancing at them for about a couple minutes each and I found issues quickly...
So just grabbing a couple native plugins off the top of my head -- 3/4th's of them had issues. So I beg you to reconsider this! You are choosing to make Flutter a less "cross" platform choice and handicapping it significantly on both platforms for anything outside of a simple application. Flutter doesn't do any good if the framework itself is awesome, if we can't do a large chunk of what we need to with it because it is impossible to interact with the Native OS properly on BOTH platforms... I understand adding Sync methods can potentially increase the chance of Deadlocks (although I believe there are a lot of already SOLVED ways to mitigate this and to eliminate that risk entirely). I also don't see why you would get any Jank; to my knowledge the rendering doesn't happen on the same thread as you use to communicate with the native side. |
|
I really do understand what you are saying. I think the most productive way for us to proceed is probably to consider each concrete case that you care about in its own bug. For example, if you want to use GVRKit with Flutter, file a bug about why you can't do so, and we can inspect those use cases individually. Maybe the solution to one case is that we need some specific features in Flutter, maybe the solution to another is that we need a first-party plugin to hide the boilerplate code, maybe the solution to another is that we have to work with the GVRKit team to have that product changed, maybe the solution to yet another is that we have to have Android itself be changed. It's hard to say, in the abstract. |
|
@Hixie - See the problem with your idea; is you still choose to totally hamstring me from the start. My tools should be trying to enable me, not cripple me by default. I maybe the odd man out here, my focus isn't on apps most the time, it is on making things work from the app to the native side. I am hired frequently to tie the two sides together and make a easy to use cross-platform interface to so that the client can type In each of my prior examples; the issue is that without the ability to talk back to Dart so that the code running in the app by the client (ie. on the Dart side) can make choice based on the event, it can't be done properly, if at all. Already Flutter is considerably harder to do a plugins than NativeScript; and was originally in my opinion about equal to React. However now finding about about this significant limitation; this really does make Flutter the worst platform to do any integration plugins on; and I've used almost all of the cross-platform platforms.
I only used GVRKit as an example; because I was working in it at that moment and I had a real case that I can see won't work in Flutter. I honestly don't need it in Flutter, Native works way better for VR/AR apps currently than any of the cross-platform platforms; and this probably won't change any time soon. So you don't need to worry about it. 😀
Synchronous message support would solve all of them. 😀
LOL, if you want the APP to respond dynamically then you still have to add a sync call back to dart; because the listener is expecting the answer when it returns out of the delegate/listener... 😀
This might work in some limited cases where you have direct ability to interact with the team. And that is making a very huge presumption that the team in question is even is responsive to your changes, and finally are they responsive? -- GVRKit releases are about a three to four month wait between any changes (closed source, no ability to do my own fix/release). I'm sure my client would be happy with me saying, yeah I choose Flutter and it was awesome; but because of X; we now have to wait for ~3 more months to launch so that Y will work in your app. Yeah, I can tell you right now that this is a HORRIBLE solution, way to many uncontrollable In addition are you really serious about working with the google maps team and have them rewrite parts of their long time established api to be fully async; as they have several sync callbacks on both Android and iOS. For example this one: Or maybe this one from a 3rd party AirBNB Lottie: Cordova/Ionic, Titanium, Xamarin, React and NativeScript can all handle all these of these issues easily and no need to wait on any "if's"... None of these are abstract issues; this is REAL deployed code and code that is used to solve real issues in real apps. Yes, I do realize neither of these plugins currently work in Flutter yet -- but if/when Flutter supports rendering of native UI inside the Skia render tree; then BOTH these plugins could be made to work by someone like me, except for the ALL the pieces that you just tied my hands...
Say you do have the power to get the Android team to add X feature as a ASYNC api; and they add it in Android 10 ; it still doesn't help you unless you are planning on ditching support for 99% of the Android devices. So unless you know something I don't -- I have a hell of a lot of devices that won't be ever running Android 10. 😀 (and then ~50% of the issues are on iOS and Apple won't ever change their delegate/event system, it is totally core to everything on Mac and iOS. ) 😀 So this really is a less valid option than even adding sync support.. 😀
You asked, you might not like the number of issues I raise that have no good solutions now... 😀 But when it comes to any customer apps; this limitation alone will pretty much cause me to nix Flutter where the client may need to do anything that isn't already built into Flutter. (Which unfortunately is pretty much all clients have one requirement like this.) The risk is now way too great that I will run into something that can't be done in Flutter and I won't be able to fix it. I've learned a long time ago that this is one of the worst places to be when dealing with paying clients waiting for you and you have no ability to solve the issue because your hands are tied. I would much rather choose a technology that may not be as good; but also won't block me. |
|
@NathanaelA One on hand I understand the concerns about bringing this API to Flutter. Chances are high that it will be abused at some point. On the other hand I've seen too many developers losing interest in Flutter because they can't interact with Android APIs as they are used to. It's not that their current patterns are the best solution, but the inability to use those is a problem. |
So long as each issue clearly describes the use case (in other words, don't just copy-and-paste a five paragraph boilerplate message into each bug and change the subject line...), the more the merrier.
Nobody's crippling your hands... this project is entirely open source. You can take the source and do whatever you want with it whenever you want. |
Ian, really? I figured with a developer of your caliper; I would NOT have to explain that statement in-depth. You are 100% correct, MY hands themselves are not tied; I'm perfectly capable of applying this patch and patching things myself, in fact I have build servers that can do a lot of this automatically -- that is not the issue. ;-) However, my hands ARE tied if I want to be able to create plugins that OTHERS in the general eco-system will use. Without this type of support I'm CANNOT create a plugin that will work with an "official" flutter engine out of the box (i.e. I'm crippled in my ability to ship it!). I have in the past tried to work out of a eco-system with plugins and it ended up being a losing cause. People loved the plugin; but without the official engine supporting it -- I literally had to rebase my patch (and fix changed patch points ) way to frequently. In effect it ties my hands because their is NO EASY WAY to distribute plugins that use this feature to the rest of the eco-system. Second this is a losing issue for the Flutter team, because when I fall behind in keeping my "patched" engine updated; then you (i.e the Flutter team) get all the bug reports that were already fixed in the current version and so you guys are having to tell people it is fixed in the latest version and/or link to the bug report as duplicates. The Flutter team ends up shouldering the majority of the support issues in this case. So in my humble opinion, forking in this scenario is typically the worst of all worlds, and my hands are tied and crippled because their is no GOOD choices... 😀 |
Agreed, and will do. 😀
Oh, I fully understand Ian's point -- and there is a serious potential to abuse this. But I honestly don't see a easy way to support the vast number of API's that require Sync in iOS and Android. Just ignoring the issue; doesn't make it vanish. All it does is make the devs that need to access these API's vanish... |
|
Note this may be revisited as part of: flutter/flutter#7053 |
|
From my perspective; Flutter is a not production ready until it has sync support; so I sincerely welcome your statement. But I honestly didn't expect to see sync support in flutter while Ian had any say in things; so your statement greatly confuses me as to how you think this will now occur... According to @Hixie:
Why would flutter/flutter#7053 make that any different? Why wouldn't C/C++ be just forced to do everything async? I honestly am not understanding why you remotely even think Ian will approve a method of Sync support into flutter when he was pretty adamant that having sync support is a "non-starter" as of several months ago... |
|
Sync callbacks do make it very easy to deadlock. They're a real foot-gun for developers, or so was our experience on the web. That said I could also believe exposing some synchronous ways to get in/out of Dart code will be required over time. Regardless, we are currently looking into a FFI interface for C++, which as I understand it would be synchronous: dart-lang/sdk#34452 |
|
Oh, I recognize the issues with Sync, and I also prefer Async calls as much as possible. But the reality is that both iOS and Android has a lot of API's that are sync that are still broke by design in flutter... However, I honestly don't understand why Ian would let Sync calls in for FFI if he believes SYNC is a non-starter... I would hate to have whoever is working on the FFI interface to waste a lot of time on SYNC support if Ian is still believing sync support is a "non-starter". That would be huge a waste of time on that developers part... That is why I was asking what makes you think sync support would be re-evaluated? |
|
The C/C++ FFI doesn't cross threads, so there's no deadlock potential. It's just about running code on the same thread as the Dart code, but written in C/C++. |
|
Do we have any plan for trapping |
Adds support for blocking message send/reply from platform code and into Flutter. This facility is needed in situations where platform-specific code receives a (synchronous) request for a value whose computation involves executing Dart code.
One example is saving UI view state on Android, cf. flutter/flutter#6827.