All termux-* result in 3 sec pegged cpu delay due to dex2oat

hanyman7 commented 1 year ago

When art is enabled and /system/bin/app_process is called it results in a dex2oat conversion prior to calling am.apk. this is because the AndroidManifest.xml file does not contain Android:vmSafeMode=true

So here is what I see out.txt

michalbednarski commented 1 year ago

On devices I tested (Android 9 and 13) that didn't seem to be case

Since am.apk is running outside Android PackageManagerService, its AndroidManifest.xml isn't used for anything (it is just there for gradle compatibility)

From what I'm seeing in Android sources, I think you'd need to change last line in script to (however I haven't really tested it to work)

exec /system/bin/app_process -Xcompiler-option --compiler-filter=assume-verified / com.termux.termuxam.Am "$@"

Also tried LD_PRELOAD-ing following, but so far not really tested that:

#include <dlfcn.h>
#include <string.h>

int property_get(const char* key, char* value, const char* default_value) {
    if (0 == strcmp(key, "vold.decrypt")) {
        strcpy(value, "trigger_restart_min_framework");
        return 29;
    }
    if (0 == strcmp(key, "dalvik.vm.perfetto_hprof")) {
        strcpy(value, "false");
        return 5;
    }
    return ((int(*)(const char* key, char* value, const char* default_value)) dlsym(RTLD_NEXT, "property_get"))(key, value, default_value);
}

dalvik.vm.perfetto_hprof has been affecting shutdown time on tested Android 13, however LD_PRELOAD wasn't working when running without debugger

hanyman7 commented 1 year ago

That didn't fix it. I still see a 3 second delay. However, been working with agnostic-apollo on a similar and have verified that modifying the am script with -Xnoimage-dex2oat fixed it.

However he wants me to test a couple of other options as well. I will try those this evening and share the results, thank you for your attention to the matter

agnostic-apollo commented 1 year ago

@michalbednarski For reference, check https://github.com/termux/termux-api/issues/552#issuecomment-1382722639

hanyman7 commented 1 year ago

Yes, I've been trying some of the suggestions/mods to the am script. So far only -Xnoimage-dex2oat has worked. I also included my comment in that thread.

twaik commented 1 year ago

Actually there is a better way to avoid this delay. There is a service tool that can send data to system services. We can rewrite termux-am tool to dlopen+dlsym libbinder and use it's API to connect ActivityManager and send data to Termux:API without invoking app_process so there will be no delay. Exact codes of requests and responses can vary from device to device so we will need to invoke app_process once in post-inst script during apt package installation.

agnostic-apollo commented 1 year ago

Have you tried that, android has restrictions on only allowing adb shell user to run am commands, similar restrictions apply to service call commands. And delay issue for termux APIs has already been solved by termux-am-socket implemented by @tareksander, which will be available in next version.

https://github.com/termux/termux-am-socket

twaik commented 1 year ago

Actually I tried it. It has same restrictions as Java Binder calls. So we can reimplement it.

agnostic-apollo commented 1 year ago

I see. Have you done performance testing?

AIDL codes do vary on devices, but there are ways to get them. Would be problematic on bootstrap installation since post install scripts are not run. Custom implementation also has advantage of custom additions and fixes.

twaik commented 1 year ago

AIDL codes do vary on devices, but there are ways to get them

Yeah, that is a reason to invoke app_process once. To extract these codes from Java part of system. We can make it run code extractor in the case if file with codes does not exist, not neccessary to make it only post-inst action.

agnostic-apollo commented 1 year ago

True. Will this be c code? How do you intend to parse the command line parameters into an Intent?

twaik commented 1 year ago

As far as I know we can simply pass Intent's parameters as is. https://cs.android.com/android/platform/superproject/+/android-11.0.0_r3:frameworks/native/cmds/service/service.cpp;l=247-323;drc=b9ec70ebb9bf4238db6f8731320a7961fffc6606

agnostic-apollo commented 1 year ago

Right, supported on Android 5/7 as well. Can be looked into in future if needed and worth it.

https://cs.android.com/android/platform/superproject/+/android-7.0.0_r1:frameworks/native/cmds/service/service.cpp;l=188

https://cs.android.com/android/platform/superproject/+/android-5.0.0_r1.0.1:frameworks/native/cmds/service/service.cpp;l=161

michalbednarski commented 1 year ago

Probably most relevant thing is to see how termux-am-socket compares if we'd want to make native version

I can think of few caveats though if you're trying to do stuff through service command though (these also need to be considered if you're building native executable talking directly with binder without using service command (either though libbinder, libbinder_ndk or direct ioctl calls to /dev/binder))

You cannot use `service` to send broadcast on Android 8

ActivityManagerService.broadcastIntent() when given no IApplicationThread (which you don't have when you're not started as app), will pass null as callerPackage
callerPackage is then passed to isInstantApp()
Until Android 9, isInstantApp throws when packageName is null and calling uid belongs to app

termux-am works that around by constructing PendingIntent and immediately sending it, but that is doing transaction on object received from another transaction, and you cannot do that with service command

On Android >= 9 that workaround is no longer necessary and commented-out broadcastIntent() version could be used

Before Android 8 you could use /system/bin/am, I don't know how that compares performance-wise though

`service intent` command is out of sync with `Intent` format

For example, on Android 5 writing Intent in service first things are action and dataArg written through writeString16. Lets compare that with actual code reading Intent in system: action is read with in.readString(), so far so good, native writeString16 matches Java Parcel.readString, next thing is data and we have mismatch, on Java side Uri needs type tag which isn't written by service command

Other things to note is absence of mPackage, mSourceBounds, mSelector and mClipData fields on service command side, as well as mismatching format of mComponent in addition of already described mismatch of mData

Looking at git log, it looks like format of Intent written by service command hasn't been changed since at least Android 4 when service command was moved into frameworks/native repository

Meanwhile on later Android versions, Parcel-ing String-s inside Intent object were switched to write/readString8, for which service command does not provide convenient way of passing (it is possible to pass them through series of i32 arguments though)

Intent object can also vary between devices, for example Samsung has additional fields related to multi-window and Link-To-Windows features

Also service intent command doesn't offer ability to pass Intent extras, which is probably necessary feature for useful am implementation

tareksander commented 1 year ago

I don't have data on termux-sm-socket, but a similar change I made to Termux:API reduced the time dramatically: https://github.com/termux/termux-api/issues/63#issuecomment-983784146

Sending an Intent to another process surely has a bit more overhead, but in Termux the libraries are all already loaded and initialized, so it should be fast. IIRC it was at least much faster than am, even without dex2oat delay. Though for easier argument passing, there is now a bash script in between the C executable for it that could impact performance, but could could be rewritten in C++ if needed.

agnostic-apollo commented 1 year ago

@michalbednarski Thanks for the indepth review. With all the various issues on the framework since for unparcel, I don't think it's worth it to go this way. And as you said, there would be device specific fields and their parceling would be build dependent. And without the extras (which I missed), an am implementation would be useless, the reason its likely not supported is because its very complex to parcel/unparcel a bundle and would be android version and device dependent.

Before Android 8 you could use /system/bin/am, I don't know how that compares performance-wise though

You can also run /system/bin/am itself on android >= 8 for specific things, just need to set up redirection.

https://github.com/termux/termux-packages/discussions/8292#discussioncomment-5102555

However, start-activity has a com.android.shell package and uid comparison check since ActivityManagerShellCommand passes SHELL_PACKAGE_NAME to ActivityManagerService. It is also passed for start-service , but it is not compared against calling uid (wonder if it has any security implications 🤔). For broadcast, SHELL_PACKAGE_NAME is not passed at all. Both start-service and broadcast seem to work on android 14.

As broadcast is used by termux-api currently and will likely use start-service in future, it can technically use /system/bin/am too, but it also starts an app_process, so not sure if there will be any significant performance difference.

`start-activity`

~ $ out="$(/system/bin/am start-activity --user "${TERMUX__USER_ID:-0}" com.termux/.app.TermuxActivity 2>&1 </dev/null)"              echo "$out"
Starting: Intent { act=android.intent.action.MAIN cat=[android.intent.category.LAUNCHER] cmp=com.termux/.app.TermuxActivity }                                           
Exception occurred while executing 'start-activity':    java.lang.SecurityException: Permission Denial: package=com.android.shell does not belong to uid=10169
        at com.android.server.wm.ActivityTaskManagerService.assertPackageMatchesCallingUid(ActivityTaskManagerService.java:2257)
        at com.android.server.wm.ActivityTaskManagerService.startActivityAsUser(ActivityTaskManagerService.java:1267)
        at com.android.server.wm.ActivityTaskManagerService.startActivityAsUser(ActivityTaskManagerService.java:1236)
        at com.android.server.am.ActivityManagerService.startActivityAsUserWithFeature(ActivityManagerService.java:3162)
        at com.android.server.am.ActivityManagerShellCommand.runStartActivity(ActivityManagerShellCommand.java:715)
        at com.android.server.am.ActivityManagerShellCommand.onCommand(ActivityManagerShellCommand.java:230)
        at com.android.modules.utils.BasicShellCommandHandler.exec(BasicShellCommandHandler.java:97)
        at android.os.ShellCommand.exec(ShellCommand.java:38)
        at com.android.server.am.ActivityManagerService.onShellCommand(ActivityManagerService.java:9725)
        at android.os.Binder.shellCommand(Binder.java:1068)
        at android.os.Binder.onTransact(Binder.java:888)
        at android.app.IActivityManager$Stub.onTransact(IActivityManager.java:5107)
        at com.android.server.am.ActivityManagerService.onTransact(ActivityManagerService.java:2650)
        at android.os.Binder.execTransactInternal(Binder.java:1344)
        at android.os.Binder.execTransact(Binder.java:1275)

`start-service`

~ $ out="$(/system/bin/am start-service --user "${TERMUX__USER_ID:-0}" com.termux/.app.TermuxService 2>&1 </dev/null)"
echo "$out"
Starting service: Intent { act=android.intent.action.MAIN cat=[android.intent.category.LAUNCHER] cmp=com.termux/.app.TermuxService }

`broadcast`

$ out="$(/system/bin/am broadcast --user "${TERMUX__USER_ID:-0}" -a com.termux.app.reload_style com.termux 2>&1 </dev/null)"
echo "$out"
Broadcasting: Intent { act=com.termux.app.reload_style flg=0x400000 pkg=com.termux }
Broadcast completed: result=0

. Though for easier argument passing, there is now a bash script in between the C executable for it that could impact performance, but could could be rewritten in C++ if needed.

Yeah, that is doable, I didn't do it when I wrote it cause bash uses lot of code and classes for parsing and I didn't have time to port.

https://github.com/termux/termux-am-socket/blob/1ac60a15c4fe52c47ed85090088525b8f317f503/termux-am.sh.in#L37

There will also be a slight improvement when I push termux-app changes for termux-am-socket where JniResult and PeerCred classes/methods jni references will be cached so that they don't need to be found of every call, not sure how much improvement that will be bring but socket multiple read/write calls may benefit.

https://github.com/termux/termux-app/blob/master/termux-shared/src/main/cpp/local-socket.cpp

tareksander commented 1 year ago

Here are some timings for am and termux-am I made in the emulator. The absolute values are irrelevant, but the ratio between the values should roughly hold true, even for real devices. I'll rewrite the bash part of termux-am in C++ and see if that improves it substantially.

$ for i in {1..10}; do (time termux-am start com.termux/.app.TermuxActivity) 2>&1 | grep real; done
real    0m0.050s
real    0m0.012s
real    0m0.015s
real    0m0.015s
real    0m0.013s
real    0m0.017s
real    0m0.015s
real    0m0.017s
real    0m0.012s
real    0m0.012s
$ for i in {1..10}; do (time am start com.termux/.app.TermuxActivity) 2>&1 | grep real; done
real    0m0.163s
real    0m0.131s
real    0m0.125s
real    0m0.124s
real    0m0.124s
real    0m0.128s
real    0m0.126s
real    0m0.124s
real    0m0.130s
real    0m0.126s

agnostic-apollo commented 1 year ago

Nice. Please also test for service and broadcasts, that's what is to be used for apis and requires higher efficiency.

tareksander commented 1 year ago

Here is the timing with my rewrite in C++:

$ for i in {1..10}; do (time ./termux-am start com.termux/.app.TermuxActivity) 2>&1 | grep real; done
real    0m0.049s
real    0m0.011s
real    0m0.008s
real    0m0.007s
real    0m0.009s
real    0m0.008s
real    0m0.011s
real    0m0.010s
real    0m0.008s
real    0m0.010s

It saves 4-5ms in the emulator.

I pushed my changes to a new bash-to-cpp branch in termux-am-socket, so you can try it. I'll replace am with the new implementation locally and try some Termux:API commands and look at the performance and see if anything breaks.

tareksander commented 1 year ago

Some more data:

# with normal am
$ for i in {1..10}; do (time termux-battery-status) 2>&1 | grep real; done
real    0m0.147s
real    0m0.122s
real    0m0.123s
real    0m0.154s
real    0m0.125s
real    0m0.127s
real    0m0.120s
real    0m0.127s
real    0m0.201s
real    0m0.184s
# with termux-am
for i in {1..10}; do (time termux-battery-status) 2>&1 | grep real; done
real    0m0.055s
real    0m0.031s
real    0m0.036s
real    0m0.037s
real    0m0.028s
real    0m0.015s
real    0m0.016s
real    0m0.014s
real    0m0.016s
real    0m0.017s
# with normal am
$ for i in {1..10}; do (time am startservice com.termux.api/.KeepAliveService) 2>&1 | grep real; done
real    0m0.125s
real    0m0.128s
real    0m0.126s
real    0m0.122s
real    0m0.126s
real    0m0.125s
real    0m0.132s
real    0m0.127s
real    0m0.122s
real    0m0.132s
# with termux-am
$ for i in {1..10}; do (time am startservice com.termux.api/.KeepAliveService) 2>&1 | grep real; done
real    0m0.017s
real    0m0.015s
real    0m0.012s
real    0m0.008s
real    0m0.010s
real    0m0.013s
real    0m0.008s
real    0m0.006s
real    0m0.010s
real    0m0.007s
#with normal am
$ for i in {1..10}; do (time termux-wake-lock) 2>&1 | grep real; done
real    0m0.171s
real    0m0.138s
real    0m0.129s
real    0m0.147s
real    0m0.130s
real    0m0.162s
real    0m0.162s
real    0m0.132s
real    0m0.141s
real    0m0.134s
#with termux-am
$ for i in {1..10}; do (time termux-wake-lock) 2>&1 | grep real; done
real    0m0.035s
real    0m0.018s
real    0m0.010s
real    0m0.009s
real    0m0.009s
real    0m0.008s
real    0m0.010s
real    0m0.008s
real    0m0.010s
real    0m0.011s

~~For some reason startservice and broadcast have way lower performance on normal am.~~

Never mind, I re-ran the am start tests and got similar results to startservice and broadcast for normal am. I probably got the data confused when copying, am is just ~10x slower in general.

agnostic-apollo commented 1 year ago

For LocalSocketManager (used for termux-am-socket server), the first issue in LocalClientSocket was that previously the InputStreamReader that reads the command arguments sent did not use a BufferedReader for the InputStreamReader, which means it was reading each character one by one from the socket with the read() call, and each call would make a JNI call, which itself is cost intensive. The writing was using a BufferedWriter for the OutputStreamWriter. Now reading will also use a BufferedReader.

https://github.com/termux/termux-app/blob/f366db0cb328ce969d70301f5a98978a29302754/termux-shared/src/main/java/com/termux/shared/shell/LocalSocketListener.java#L68

https://github.com/termux/termux-app/blob/2aa7f43d1c7739ab57fe50669f84588daf1602d7/termux-shared/src/main/java/com/termux/shared/net/socket/local/LocalClientSocket.java#L189

However, just using a BufferedReader or BufferedWriter will not just improve spreads, the correct implementation for the InputStream and OutputStream is required as well.

Previously, the read() and read(bytes[] b) methods for InputStream were implemented/overridden, instead of read(byte b[], int off, int len). The default implementation of read(bytes[] b) just calls read(b, 0, b.length).

And the write() and write(bytes[] b) methods for OutputStream were implemented/overridden, instead of write(byte b[], int off, int len). The default implementation of write(bytes[] b) just calls write(b, 0, b.length).

But if you look at the implementation of BufferedReader.read(), it directly calls InputStream.read(byte b[], int off, int len) instead of read(bytes[] b) and implementation of BufferedWriter.write(String str) directly calls OutputStream.write(byte b[], int off, int len) instead of write(bytes[] b), essentially making the overriding of read(bytes[] b) and write(bytes[] b) methods useless and technically they are just wrappers, so shouldn't be overridden in the first place.

What this means is that despite usage of BufferedReader and BufferedWriter, any data read or sent to the client would be done ONE BYTE AT A TIME. For termux-am-socket, with only few bytes sent as arguments, it wouldn't matter much, but a lot of data may be returned by the server for a command. For example termux-am-socket --am-help returns the help output of 4618 bytes, which means that 4618 JNI calls and then system calls would be made, which of course is highly inefficient and was causing a 60ms delay (see drop from 71ms to 11ms below). Both read and write will now use a 512 byte buffer, instead of the default 8192 buffer used by BufferedReader and BufferedWriter, since normally am will not receive or send too much data.

Moreover, for certain commands, lot of bytes may be sent as arguments too, and LocalSocketManager is an abstract implementation of filesystem sockets and a server different than AmSocketServer may have lot of read/writes between server and client, which would be increasing latencies.

Cached classes below refers to caching of JniResult and PeerCred classes and their methods. Additionally, jstring logTitle is no longer passed to read(), send() and available() JNI methods and an int is returned for the methods instead of JniResult, however, no improvement was seen for termux-app-socket since only a single read() and write() call is made for a large enough buffer, so any performance difference would be negligible, however changes are still done for other kinds of servers.

https://github.com/termux/termux-app/blob/2aa7f43d1c7739ab57fe50669f84588daf1602d7/termux-shared/src/main/cpp/local-socket.cpp#L351

Poco F2 Pro (Snapdragon 820, Android 11)

# am
$ count=100; t1=$(date +%s%3N); for i in $(seq $count); do am --help &>/dev/null; done; t2=$(date +%s%3N); echo "$(((t2-t1)/count)) ms"
586 ms

$ count=100; t1=$(date +%s%3N); for i in $(seq $count); do am broadcast -a com.termux.app.test_action --es par1 123456789 --es abcdefghijklmnopqrstuvvxyz com.termux 1>/dev/null; done; t2=$(date +%s%3N); echo "$(((t2-t1)/count)) ms"
587 ms

# termux-am-socket - Current implementation
$ count=100; t1=$(date +%s%3N); for i in $(seq $count); do termux-am --am-help 1>/dev/null; done; t2=$(date +%s%3N); echo "$(((t2-t1)/count)) ms"
92 ms

$ count=100; t1=$(date +%s%3N); for i in $(seq $count); do termux-am broadcast -a com.termux.app.test_action --es par1 123456789 --es abcdefghijklmnopqrstuvvxyz com.termux 1>/dev/null; done; t2=$(date +%s%3N); echo "$(((t2-t1)/count)) ms"
24 ms

# termux-am-socket - Cache classes
$ count=100; t1=$(date +%s%3N); for i in $(seq $count); do termux-am --am-help 1>/dev/null; done; t2=$(date +%s%3N); echo "$(((t2-t1)/count)) ms"
71 ms

$ count=100; t1=$(date +%s%3N); for i in $(seq $count); do termux-am broadcast -a com.termux.app.test_action --es par1 123456789 --es abcdefghijklmnopqrstuvvxyz com.termux 1>/dev/null; done; t2=$(date +%s%3N); echo "$(((t2-t1)/count)) ms"
15ms

# termux-am-socket - BufferedReader and correctly implemented InputStream/OutputStream
$ count=100; t1=$(date +%s%3N); for i in $(seq $count); do termux-am --am-help 1>/dev/null; done; t2=$(date +%s%3N); echo "$(((t2-t1)/count)) ms"
11 ms

$ count=100; t1=$(date +%s%3N); for i in $(seq $count); do termux-am broadcast -a com.termux.app.test_action --es par1 123456789 --es abcdefghijklmnopqrstuvvxyz com.termux 1>/dev/null; done; t2=$(date +%s%3N); echo "$(((t2-t1)/count)) ms"
12 ms

LG G5 (Snapdragon 820, Android 7)

# am
$ count=100; t1=$(date +%s%3N); for i in $(seq $count); do am --help &>/dev/null; done; t2=$(date +%s%3N); echo "$(((t2-t1)/count)) ms"
1098 ms

$ count=100; t1=$(date +%s%3N); for i in $(seq $count); do am broadcast -a com.termux.app.test_action --es par1 123456789 --es abcdefghijklmnopqrstuvvxyz com.termux 1>/dev/null; done; t2=$(date +%s%3N); echo "$(((t2-t1)/count)) ms"
1097 ms

# termux-am-socket - Current implementation
$ count=100; t1=$(date +%s%3N); for i in $(seq $count); do termux-am --am-help 1>/dev/null; done; t2=$(date +%s%3N); echo "$(((t2-t1)/count)) ms"
154 ms

$ count=100; t1=$(date +%s%3N); for i in $(seq $count); do termux-am broadcast -a com.termux.app.test_action --es par1 123456789 --es abcdefghijklmnopqrstuvvxyz com.termux 1>/dev/null; done; t2=$(date +%s%3N); echo "$(((t2-t1)/count)) ms"
48 ms

# termux-am-socket - Cache classes + BufferedReader and correctly implemented InputStream/OutputStream
$ count=100; t1=$(date +%s%3N); for i in $(seq $count); do termux-am --am-help 1>/dev/null; done; t2=$(date +%s%3N); echo "$(((t2-t1)/count)) ms"
37 ms

$ count=100; t1=$(date +%s%3N); for i in $(seq $count); do termux-am broadcast -a com.termux.app.test_action --es par1 123456789 --es abcdefghijklmnopqrstuvvxyz com.termux 1>/dev/null; done; t2=$(date +%s%3N); echo "$(((t2-t1)/count)) ms"
40 ms

I pushed my changes to a new bash-to-cpp branch in termux-am-socket, so you can try it

Thanks for that. I'll look into it later, although I am pretty sure it would be more complicated than couple of lines of code to parse into arguments, will have to be looked into.

tareksander commented 1 year ago

The escaping is a bit more complicated than I thought, can we drop the single quote support for the server tokenizer? Double quotes with escapes work just as well, and that way you can also embed quotes.

agnostic-apollo commented 1 year ago

No, termux-am is meant to be a full replacement of am and users may pass any form of arguments and single quotes are very common. A slower but fully working implementation is better than a broken one. A full port for parsing can be done in future when it can be properly done.

tareksander commented 1 year ago

I don't think anyone will use netcat or something like that to manually communicate with the server, so the internal protocol isn't that important. The problem with single quotes is that they currently would require 2 passes: if there is whitespace in the argument, it has to be surrounded in double quotes, in which case single quotes don't need to be escaped. If there is no whitespace, freestanding single quotes have to be escaped. I tried surrounding every argument with double quotes, but that didn't work with the server for some reason, am works with that though, so that'll have to be looked into anyways.

You can test it anyways and see if writing a proper implementation in C++ is even worth it.

hanyman7 commented 1 year ago

Can anyone explain why there is an extra 300mA drain on the battery that always causes termux-battery-status to read 300mA less than the actual value... ?!?! I wrote an apk to monitor battery charge, discharge with projections etc, and the readings from the apk are ALWAYS about 300mA higher.

I do see a slight decline in amperage reading in the apk while running the termux API command and this happens regardless of charging or discharge and/or whether I apply the am patch.

It almost feels like we are taxing the heck out of the CPU momentarily so much so that by the time we get to read battery status, the results are WRONG...

Any ideas ? Should I open another defect or will the future fix not cause as much battery drain ?

Btw running on Galaxy A54 which doesn't have the need for the am patch

agnostic-apollo commented 1 year ago

So Android 14 has broken am broadcast. There is an internal check for if IIntentReceiver resultTo.asBinder() is an instanceof BinderProxy, then an exception is thrown and command hangs forever since TermuxAm never receives a callback for performReceive(). Exception methods and sources are not available currently, they are recent changes for Android 14. However, /system/bin/am works fine, even though it is using a similar implementation. Not sure how to fix it. I could redirect to /system/bin/am instead on Android 14 (or >= 9) for broadcast commands. The startactivity and startservice do seem to work though for basic testing.

ActivityManager system_server E Sending broadcast com.termux.app.test_action with resultTo requires resultToApp
java.lang.Throwable
    at com.android.server.am.ActivityManagerService.broadcastIntentLockedTraced(ActivityManagerService.java:14394)
    at com.android.server.am.ActivityManagerService.broadcastIntentLocked(ActivityManagerService.java:14351)
    at com.android.server.am.ActivityManagerService.broadcastIntentInPackage(ActivityManagerService.java:15344)
    at com.android.server.am.ActivityManagerService$LocalService.broadcastIntentInPackage(ActivityManagerService.java:18044)
    at com.android.server.am.PendingIntentRecord.sendInner(PendingIntentRecord.java:591)
    at com.android.server.am.PendingIntentRecord.send(PendingIntentRecord.java:318)
    at android.content.IIntentSender$Stub.onTransact(IIntentSender.java:99)
    at android.os.Binder.execTransactInternal(Binder.java:1344)
    at android.os.Binder.execTransact(Binder.java:1275)

Also noticed another warning exception before the above for the PendingIntent variant, not the commented out mBroadcastIntent. Again, changes are not available.

https://cs.android.com/android/platform/superproject/+/android-13.0.0_r54:frameworks/base/services/core/java/com/android/server/am/PendingIntentRecord.java;l=342

ActivityManager system_server W Sending of null from 1000 requested resultTo without an IApplicationThread!
java.lang.Throwable
    at com.android.server.am.PendingIntentRecord.sendInner(PendingIntentRecord.java:540)
    at com.android.server.am.PendingIntentRecord.send(PendingIntentRecord.java:318)
    at android.content.IIntentSender$Stub.onTransact(IIntentSender.java:99)
    at android.os.Binder.execTransactInternal(Binder.java:1344)
    at android.os.Binder.execTransact(Binder.java:1275)

android.content.IIntentReceiver.Stub implementation in Android 14 framework.jar

```java package android.content; import android.os.Binder; import android.os.Bundle; import android.os.IBinder; import android.os.IInterface; import android.os.Parcel; import android.os.RemoteException; public interface IIntentReceiver extends IInterface { void performReceive(Intent intent, int i, String str, Bundle bundle, boolean z, boolean z2, int i2) throws RemoteException; public static class Default implements IIntentReceiver { @Override // android.content.IIntentReceiver public void performReceive(Intent intent, int resultCode, String data, Bundle extras, boolean ordered, boolean sticky, int sendingUser) throws RemoteException { } @Override // android.os.IInterface public IBinder asBinder() { return null; } } public static abstract class Stub extends Binder implements IIntentReceiver { public static final String DESCRIPTOR = "android.content.IIntentReceiver"; static final int TRANSACTION_performReceive = 1; public Stub() { attachInterface(this, DESCRIPTOR); } public static IIntentReceiver asInterface(IBinder obj) { if (obj == null) { return null; } IInterface iin = obj.queryLocalInterface(DESCRIPTOR); if (iin != null && (iin instanceof IIntentReceiver)) { return (IIntentReceiver) iin; } return new Proxy(obj); } @Override // android.os.IInterface public IBinder asBinder() { return this; } public static String getDefaultTransactionName(int transactionCode) { switch (transactionCode) { case 1: return "performReceive"; default: return null; } } @Override // android.os.Binder public String getTransactionName(int transactionCode) { return getDefaultTransactionName(transactionCode); } @Override // android.os.Binder public boolean onTransact(int code, Parcel data, Parcel reply, int flags) throws RemoteException { if (code >= 1 && code <= 16777215) { data.enforceInterface(DESCRIPTOR); } switch (code) { case IBinder.INTERFACE_TRANSACTION /* 1598968902 */: reply.writeString(DESCRIPTOR); return true; default: switch (code) { case 1: Intent _arg0 = (Intent) data.readTypedObject(Intent.CREATOR); int _arg1 = data.readInt(); String _arg2 = data.readString(); Bundle _arg3 = (Bundle) data.readTypedObject(Bundle.CREATOR); boolean _arg4 = data.readBoolean(); boolean _arg5 = data.readBoolean(); int _arg6 = data.readInt(); data.enforceNoDataAvail(); performReceive(_arg0, _arg1, _arg2, _arg3, _arg4, _arg5, _arg6); return true; default: return super.onTransact(code, data, reply, flags); } } } public static class Proxy implements IIntentReceiver { private IBinder mRemote; Proxy(IBinder remote) { this.mRemote = remote; } @Override // android.os.IInterface public IBinder asBinder() { return this.mRemote; } public String getInterfaceDescriptor() { return Stub.DESCRIPTOR; } @Override // android.content.IIntentReceiver public void performReceive(Intent intent, int resultCode, String data, Bundle extras, boolean ordered, boolean sticky, int sendingUser) throws RemoteException { Parcel _data = Parcel.obtain(asBinder()); try { _data.writeInterfaceToken(Stub.DESCRIPTOR); _data.writeTypedObject(intent, 0); _data.writeInt(resultCode); _data.writeString(data); _data.writeTypedObject(extras, 0); _data.writeBoolean(ordered); _data.writeBoolean(sticky); _data.writeInt(sendingUser); this.mRemote.transact(1, _data, null, 1); } finally { _data.recycle(); } } } @Override // android.os.Binder public int getMaxTransactionId() { return 0; } } } ```

CC @michalbednarski

agnostic-apollo commented 1 year ago

I don't think anyone will use netcat or something like that to manually communicate with the server, so the internal protocol isn't that important.

Yeah, true.

One could technically surround each argument in single quotes and escape any single quotes inside the arguments themselves. That is what is recommended for termux-tasker arguments, also parsed by ArgumentTokenizer. Args in single quotes are taken literally, so no other processing required.

https://github.com/termux/termux-tasker/blob/master/app/src/main/java/com/termux/tasker/FireReceiver.java#L176

https://github.com/termux/termux-tasker/tree/master#arguments

agnostic-apollo commented 1 year ago

Can anyone explain why there is an extra 300mA drain on the battery that always causes termux-battery-status to read 300mA less than the actual value... ?!?!

The value for current returned is for instantaneous battery current in microamperes (note that not in milliamperes). And termux-api starts a new app process, so there is bound to be some CPU bump, etc, causing more current being drawn from the battery. 300000 isn't that much. And there can be other stuff going on in the background, so such measurements shouldn't be relied upon directly without averaging over a timespan.

https://github.com/termux/termux-api/blob/d2f205113e55b344980bceff8f061e873f996d27/app/src/main/java/com/termux/api/apis/BatteryStatusAPI.java#L113C89-L113C117

https://developer.android.com/reference/android/os/BatteryManager#BATTERY_PROPERTY_CURRENT_NOW

hanyman7 commented 1 year ago

In general I agree with you but something is off. What I am observing is a consistent reading of about 300ma below true consumption, or charging, being it instantaneous or average. e.g. my battery.apk updates every second and bumps up and down and averages at -150ma I put termux-battery-status in a sleep / read loop and measure between a 450 to 500 ma consumption. So even sampling is off ??? In my apk, I use the instantaneous value, current percent and battery capacity to predict time to empty or full charge. The apk numbers come out fairly accurate, however a similar calculation in termux is always off.

sylirre commented 1 year ago

termux-battery-status doesn't do sampling. It returns instant value retrieved by

batteryManager.getLongProperty(BatteryManager.BATTERY_PROPERTY_CURRENT_NOW)

Moreover every call of termux-battery-status will give a CPU usage spike and increase the current. There is a difference between your app which constantly running and termux-battery-status script which basically launches activity manager java application (am) and Termux:API Android application whenever you execute the command.

hanyman7 commented 1 year ago

I understand that. The only reason I mentioned sampling was in response to Apollo, in that it can't be used for that and I know that it doesn't do that because I did look at the source.

That being said, I would agree with you that the call itself produces such a spike because of all of the subsequent Socket, TermuxAPI and starting a jvm.

The net though, I am a user. My phone consumes 150mA while not charging. I ask termux what my battery status is and part of the response is 450mA. All the other status info is correct, but we have a case of ... " the result is different because you are looking " .... Wouldn't you agree ? I could understand a small difference, but the answer is 3x off.

Just a thought 🤔... Would it be overkill to allow the user to pass a sleep value to overcome the spike ? That could be a Thread sleep prior to BatteryManager reading the current. It would allow for a much more accurate reading.

agnostic-apollo commented 1 year ago

If you are running github builds, try running termux-api-start before running commands. If it were a foreground service, then would be better, but current service should help too to reduce app startups each time if in short timespan.

https://github.com/termux/termux-api-package/commit/6f8a41eaf924e7704cd22edacdc28b5fac80c210

https://github.com/termux/termux-api/commit/4c8296e93881a30871155aa25af03d46f429085d

tareksander commented 1 year ago

I fixed the argument quoting for the C++ rewrite (hopefully for real this time), and changed it a bit to more exactly match the old termux-am bash script in behavior. When compiling you can uncomment these lines to test just the argument quoting without actually calling the am server. I'm using this file to check how the server parses the string btw.

termux / TermuxAm