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.

Sign up for GitHub

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

Jump to bottom

Add ACTION_SERVICE_STOP intent to only stop a single AppShell #3821

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add ACTION_SERVICE_STOP intent to only stop a single AppShell #3821

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
e3c8598
Add ACTION_SERVICE_STOP intent to only stop a single AppShell
lvogt Jan 28, 2024
e5299db
Fix review findings
lvogt Feb 5, 2024
81fb1c1
Fix review findings #2
lvogt Feb 9, 2024
caf2100
Fix comment
lvogt Feb 9, 2024
7d3ff3e
Rename constant
lvogt Feb 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
22 changes: 22 additions & 0 deletions app/src/main/java/com/termux/app/TermuxService.java
View file
Open in desktop
Expand Up @@ -8,6 +8,7 @@
import android.content.Context;
import android.content.Intent;
import android.content.res.Resources;
import android.net.Uri;
import android.net.wifi.WifiManager;
import android.os.Binder;
import android.os.Build;
Expand Down Expand Up @@ -154,6 +155,10 @@ public int onStartCommand(Intent intent, int flags, int startId) {
Logger.logDebug(LOG_TAG, "ACTION_SERVICE_EXECUTE intent received");
actionServiceExecute(intent);
break;
case TERMUX_SERVICE.ACTION_SERVICE_STOP:
Logger.logDebug(LOG_TAG, "ACTION_SERVICE_STOP intent received");
actionServiceStop(intent);
break;
default:
Logger.logError(LOG_TAG, "Invalid action: \"" + action + "\"");
break;
Expand Down Expand Up @@ -354,6 +359,23 @@ private void actionReleaseWakeLock(boolean updateNotification) {
Logger.logDebug(LOG_TAG, "WakeLocks released successfully");
}

private void actionServiceStop(Intent intent) {
if (intent == null) {
Logger.logError(LOG_TAG, "Ignoring null intent to actionServiceStop");
return;
}

int gracePeriod = IntentUtils.getIntegerExtraIfSet(intent, TERMUX_SERVICE.EXTRA_TERMINATE_GRACE_PERIOD, 5000);

Uri executableUri = intent.getData();
String executable = UriUtils.getUriFilePathWithFragment(executableUri);
String shellName = ShellUtils.getExecutableBasename(executable);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There would be conflicts if same executable was run from someplace else, and that function only returns first one. You should generate at least a six character alphanumeric [a-zA-Z0-9] random string and store that in cron tab in addition to the id and when sending execute or stop intent, pass that as TERMUX_SERVICE.EXTRA_SHELL_NAME, so that the executable for the job is killed, and not any other.

d287734

#3709

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point.
Is there a character limit on the shell name?
Or the other way around: Could I just use a UUID?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no limit currently, but there should be, ideally 255, equal to NAME_NAME.

UUID would be too long and harder to see in app shells UI list and logs, just use a six character alphanumeric [a-zA-Z0-9] string, like mktemp template uses for unique files. Following should work, make sure <script_name>_<public_id>_<private_id> doesn't already exist in cron tab, otherwise regenerate private_id. The public_id here is the incrementing number you are currently using.

char[] allowedCharsArray = ("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789").toCharArray();
char[] private_id = new char[6];
Random random = new SecureRandom();
for (int i = 0; i < private_id.length; i++) {
    private_id[i] = allowedCharsArray[random.nextInt(allowedCharsArray.length)];
}

https://man7.org/linux/man-pages/man3/mktemp.3.html

https://bugs.python.org/issue12015

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Thanks for the example.

I changed the method to only work if the shell name is explicitly set.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Welcome. Makes sense. I think it would be better to call getTermuxTaskForShellName() in a while loop until it starts returning null, so that all shells with same name get killed, since currently only first one would get killed, which may not be the one caller intended to kill. To prevent duplicates, callers should start shells with a unique shell name, like the cron API would do.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point - I had not considered multiple shells with the same name.

AppShell appShell = getTermuxTaskForShellName(shellName);
if (appShell != null) {
appShell.terminateIfExecuting(getApplicationContext(), gracePeriod, true);
}
}

/** Process {@link TERMUX_SERVICE#ACTION_SERVICE_EXECUTE} intent to execute a shell command in
* a foreground TermuxSession or in a background TermuxTask. */
private void actionServiceExecute(Intent intent) {
Expand Down
46 changes: 46 additions & 0 deletions termux-shared/src/main/java/com/termux/shared/shell/command/runner/app/AppShell.java
View file
Open in desktop
@@ -1,6 +1,7 @@
package com.termux.shared.shell.command.runner.app;

import android.content.Context;
import android.os.Handler;
import android.system.ErrnoException;
import android.system.Os;
import android.system.OsConstants;
Expand Down Expand Up @@ -245,6 +246,37 @@ private void executeInner(@NonNull final Context context) throws IllegalThreadSt
AppShell.processAppShellResult(this, null);
}

/**
* Terminate this {@link AppShell} by sending a {@link OsConstants#SIGTERM} to its {@link #mProcess}
* if it is still executing. After {@code gracePeriodMsec} milliseconds {@link OsConstants#SIGTERM} is
* signalled.
*
* @param context The {@link Context} for operations.
* @param gracePeriodMsec The delay after which a SIGKILL is send.
* @param processResult If set to {@code true}, then the {@link #processAppShellResult(AppShell, ExecutionCommand)}
* will be called to process the failure.
*/
public void terminateIfExecuting(@NonNull final Context context, long gracePeriodMsec, boolean processResult) {
if (gracePeriodMsec == 0) {
killIfExecuting(context, processResult);
return;
}

// If execution command has already finished executing, then no need to process results or sending any signals
if (mExecutionCommand.hasExecuted()) {
Logger.logDebug(LOG_TAG, "Ignoring sending SIGTERM or SIGKILL to \"" + mExecutionCommand.getCommandIdAndLabelLogString() + "\" AppShell since it has already finished executing");
return;
}

Logger.logDebug(LOG_TAG, "Send SIGTERM to \"" + mExecutionCommand.getCommandIdAndLabelLogString() + "\" AppShell");

if (mExecutionCommand.isExecuting()) {
term();
}

(new Handler()).postDelayed(() -> killIfExecuting(context, processResult), gracePeriodMsec);
}

/**
* Kill this {@link AppShell} by sending a {@link OsConstants#SIGILL} to its {@link #mProcess}
* if its still executing.
Expand Down Expand Up @@ -274,6 +306,20 @@ public void killIfExecuting(@NonNull final Context context, boolean processResul
}
}


/**
* Terminate this {@link AppShell} by sending a {@link OsConstants#SIGTERM} to its {@link #mProcess}.
*/
public void term() {
int pid = ShellUtils.getPid(mProcess);
try {
// Send SIGKILL to process
Os.kill(pid, OsConstants.SIGTERM);
} catch (ErrnoException e) {
Logger.logWarn(LOG_TAG, "Failed to send SIGTERM to \"" + mExecutionCommand.getCommandIdAndLabelLogString() + "\" AppShell with pid " + pid + ": " + e.getMessage());
}
}

/**
* Kill this {@link AppShell} by sending a {@link OsConstants#SIGILL} to its {@link #mProcess}.
*/
Expand Down
6 changes: 6 additions & 0 deletions termux-shared/src/main/java/com/termux/shared/termux/TermuxConstants.java
View file
Open in desktop
Expand Up @@ -993,6 +993,9 @@ public static final class TERMUX_SERVICE {
/** Intent action to execute command with TERMUX_SERVICE */
public static final String ACTION_SERVICE_EXECUTE = TERMUX_PACKAGE_NAME + ".service_execute"; // Default: "com.termux.service_execute"

/** Intent action to execute command with TERMUX_SERVICE */
public static final String ACTION_SERVICE_STOP = TERMUX_PACKAGE_NAME + ".service_execution_stop"; // Default: "com.termux.service_execute"
agnostic-apollo marked this conversation as resolved.
Show resolved Hide resolved

/** Uri scheme for paths sent via intent to TERMUX_SERVICE */
public static final String URI_SCHEME_SERVICE_EXECUTE = TERMUX_PACKAGE_NAME + ".file"; // Default: "com.termux.file"
/** Intent {@code String[]} extra for arguments to the executable of the command for the TERMUX_SERVICE.ACTION_SERVICE_EXECUTE intent */
Expand Down Expand Up @@ -1046,6 +1049,9 @@ public static final class TERMUX_SERVICE {
* be created in {@link #EXTRA_RESULT_DIRECTORY} if {@link #EXTRA_RESULT_SINGLE_FILE} is
* {@code false} for the TERMUX_SERVICE.ACTION_SERVICE_EXECUTE intent */
public static final String EXTRA_RESULT_FILES_SUFFIX = TERMUX_PACKAGE_NAME + ".execute.result_files_suffix"; // Default: "com.termux.execute.result_files_suffix"
/** Intent {@code long} extra for graceperiod between SIGTERM and SIGKILL
* for the TERMUX_SERVICE.ACTION_SERVICE_STOP intent */
public static final String EXTRA_TERMINATE_GRACE_PERIOD = TERMUX_PACKAGE_NAME + ".execute.stop.delay";



Expand Down