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

Arrange the timeout logic and values. #238

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
+82 −16
Open

Arrange the timeout logic and values. #238

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
87d709b
Arrange the timeout logic and values.
jfclere Jan 28, 2025
8542b76
Describe the new behaviour.
jfclere Feb 6, 2025
d425c5f
Add defines for 15 seconds and 15000 ms.
jfclere Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 57 additions & 12 deletions src/native/windows/apps/prunsrv/prunsrv.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,9 @@
#define STDOUT_FILENO 1
#define STDERR_FILENO 2
#define ONE_MINUTE (60 * 1000)
#define ONE_MINUTE_SEC 60
#define TIMEFORSERVICEMANAGER 15
#define MILLITIMEFORSERVICEMANAGER 15000

#ifdef _WIN64
#define KREG_WOW6432 KEY_WOW64_32KEY
Expand Down Expand Up @@ -952,9 +955,16 @@ static BOOL docmdStopService(LPAPXCMDLINE lpCmdline)
if (!rv) {
/* Wait for the timeout if any */
int timeout = SO_STOPTIMEOUT;
if (timeout == 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not <= here?

/* waiting for ever doesn't look OK here */
timeout = ONE_MINUTE_SEC;
}
if (timeout) {
Copy link
Contributor

Choose a reason for hiding this comment

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

timeout cannot be zero here this this will always be true. Therefore, no need for the if statement.

/* the SO_STOPTIMEOUT applies to the stop command and to the time service needs to stop */
/* We also add TIMEFORSERVICEMANAGER seconds for the service logic */
int i;
for (i = 0; i < timeout; i++) {
for (i = 0; i < (timeout*2+TIMEFORSERVICEMANAGER); i++) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This makes the minimum timeout 17s. That seems high to me.

I would expect to set a timeout for the total time (sending the stop command, waiting for the service to stop, overhead). How practical is to set set a maximumFinishTime here of currentTime + timeout and then calculate the timeout as maximumFinishTime - currentTime whenever a timeout is required?

/* apxServiceCheckStop waits 1000 ms */
rv = apxServiceCheckStop(hService);
apxLogWrite(APXLOG_MARK_DEBUG "apxServiceCheck returns %d.", rv);
if (rv)
Expand Down Expand Up @@ -1230,8 +1240,6 @@ static DWORD WINAPI serviceStop(LPVOID lpParameter)
apxLogWrite(APXLOG_MARK_INFO "Worker is not defined.");
return TRUE; /* Nothing to do */
}
if (timeout > 0x7FFFFFFF)
timeout = INFINITE; /* If the timeout was '-1' wait forewer */
if (_jni_shutdown) {
if (!IS_VALID_STRING(SO_STARTPATH) && IS_VALID_STRING(SO_STOPPATH)) {
/* If the Working path is specified change the current directory
Expand Down Expand Up @@ -1269,7 +1277,7 @@ static DWORD WINAPI serviceStop(LPVOID lpParameter)
}
else {
if (lstrcmpA(_jni_sclass, "java/lang/System") == 0) {
reportServiceStatus(SERVICE_STOP_PENDING, NO_ERROR, 20 * 1000);
reportServiceStatus(SERVICE_STOP_PENDING, NO_ERROR, ONE_MINUTE);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this hard-coded? Shouldn't it depend on the timeout?

apxLogWrite(APXLOG_MARK_DEBUG "Forcing Java JNI System.exit() worker to finish...");
return 0;
}
Expand Down Expand Up @@ -1380,7 +1388,10 @@ static DWORD WINAPI serviceStop(LPVOID lpParameter)

if (dwCtrlType == SERVICE_CONTROL_SHUTDOWN)
timeout = MIN(timeout, apxGetMaxServiceTimeout(gPool));
reportServiceStatus(SERVICE_STOP_PENDING, NO_ERROR, timeout);
if (timeout)
reportServiceStatus(SERVICE_STOP_PENDING, NO_ERROR, timeout);
else
reportServiceStatus(SERVICE_STOP_PENDING, NO_ERROR, ONE_MINUTE_SEC);

if (timeout) {
FILETIME fts, fte;
Copy link
Contributor

Choose a reason for hiding this comment

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

Again, isn't timeout now always >0 ?

Expand Down Expand Up @@ -1609,11 +1620,13 @@ void WINAPI service_ctrl_handler(DWORD dwCtrlCode)
case SERVICE_CONTROL_STOP:
apxLogWrite(APXLOG_MARK_INFO "Service SERVICE_CONTROL_STOP signalled.");
_exe_shutdown = TRUE;
if (SO_STOPTIMEOUT > 0) {
reportServiceStatus(SERVICE_STOP_PENDING, NO_ERROR, SO_STOPTIMEOUT * 1000);
/* What hint value should we give 2*TIMEOUT+15 in seconds from docmdStopService() and tests */
if (SO_STOPTIMEOUT) {
reportServiceStatus(SERVICE_STOP_PENDING, NO_ERROR, SO_STOPTIMEOUT * 2000 + MILLITIMEFORSERVICEMANAGER);
}
else {
reportServiceStatus(SERVICE_STOP_PENDING, NO_ERROR, 3 * 1000);
/* Use 1 minutes default */
reportServiceStatus(SERVICE_STOP_PENDING, NO_ERROR, ONE_MINUTE * 2 + MILLITIMEFORSERVICEMANAGER);
}
/* Stop the service asynchronously */
stopThread = CreateThread(NULL, 0,
Expand Down Expand Up @@ -1680,6 +1693,14 @@ void WINAPI serviceMain(DWORD argc, LPTSTR *argv)

apxLogWrite(APXLOG_MARK_DEBUG "Inside serviceMain()...");

/* DWORD is typedef unsigned long DWORD; */
if (SO_STOPTIMEOUT > 0x7FFFFFFF) {
/* apxGetMaxServiceTimeout is in ms */
SO_STOPTIMEOUT = apxGetMaxServiceTimeout(gPool) / 1000;
if (SO_STOPTIMEOUT > 0x7FFFFFFF)
SO_STOPTIMEOUT = ONE_MINUTE_SEC; /* Assume one minute */
}

if (IS_VALID_STRING(_service_name)) {
WCHAR en[SIZ_HUGLEN];
int i;
Expand Down Expand Up @@ -1838,8 +1859,9 @@ void WINAPI serviceMain(DWORD argc, LPTSTR *argv)
SetConsoleCtrlHandler((PHANDLER_ROUTINE)console_handler, TRUE);

if (SO_STOPTIMEOUT) {
/* we have a stop timeout */
/* we wait for ever for the service to be done, printing a debug message every 2 seconds */
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this wait for timeout seconds rather than waiting forever?

BOOL bLoopWarningIssued = FALSE;
DWORD count = 0;
do {
/* wait 2 seconds */
DWORD rv = apxHandleWait(gWorker, 2000, FALSE);
Expand All @@ -1849,11 +1871,21 @@ void WINAPI serviceMain(DWORD argc, LPTSTR *argv)
bLoopWarningIssued = TRUE;
}
Sleep(2000);
count = count + 2;
if (count >= SO_STOPTIMEOUT) {
apxLogWrite(APXLOG_MARK_WARN "waited %d sec, Timeout reached!" , count);
break;
}
}
if (rv == WAIT_OBJECT_0)
apxLogWrite(APXLOG_MARK_DEBUG "Worker has crashed or stopped!");
else
apxLogWrite(APXLOG_MARK_DEBUG "waiting until Worker is done...");
} while (!_exe_shutdown);
apxLogWrite(APXLOG_MARK_DEBUG "waiting %d sec... shutdown: %d", SO_STOPTIMEOUT, _exe_shutdown);
apxHandleWait(gWorker, SO_STOPTIMEOUT*1000, FALSE);
} else {
apxLogWrite(APXLOG_MARK_DEBUG "waiting until Worker is done...");
apxHandleWait(gWorker, INFINITE, FALSE);
}
apxLogWrite(APXLOG_MARK_DEBUG "Worker finished.");
Expand All @@ -1876,8 +1908,21 @@ void WINAPI serviceMain(DWORD argc, LPTSTR *argv)
*/
apxLogWrite(APXLOG_MARK_DEBUG "Waiting 1 minute for all threads to exit.");
reportServiceStatus(SERVICE_STOP_PENDING, NO_ERROR, ONE_MINUTE);
apxDestroyJvm(ONE_MINUTE);
/* if we are not using JAVA apxDestroyJvm does nothing, check the chid processes in case they hang */
if (!apxDestroyJvm(ONE_MINUTE)) {
/* if we are not using JAVA apxDestroyJvm does nothing, check the chid processes in case they hang */
apxLogWrite(APXLOG_MARK_DEBUG "Not using JAVA apxDestroyJvm did nothing");
DWORD count = SO_STOPTIMEOUT;
if (!count)
count = ONE_MINUTE_SEC;
do {
Comment on lines +1914 to +1917
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like a possible 3rd wait using the stop timeout.

if (!apxProcessTerminateChild( GetCurrentProcessId(), TRUE)) {
Sleep(1000);
count = count - 1;
} else {
break;
}
} while (count);
}
apxProcessTerminateChild( GetCurrentProcessId(), FALSE); /* FALSE kills! */
}
else {
Expand All @@ -1887,7 +1932,7 @@ void WINAPI serviceMain(DWORD argc, LPTSTR *argv)
*/
apxLogWrite(APXLOG_MARK_DEBUG "Waiting for all threads to exit.");
apxDestroyJvm(INFINITE);
reportServiceStatus(SERVICE_STOP_PENDING, NO_ERROR, 0);
reportServiceStatus(SERVICE_STOP_PENDING, NO_ERROR, ONE_MINUTE);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why hard-coded to a minute?

}
apxLogWrite(APXLOG_MARK_DEBUG "JVM destroyed.");
reportServiceStatusStopped(apxGetVmExitCode());
Expand Down
4 changes: 3 additions & 1 deletion src/native/windows/src/javajni.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,8 +486,10 @@ apxDestroyJvm(DWORD dwTimeout)
CloseHandle(hWaiter);
return rv;
}
else
else {
apxLogWrite(APXLOG_MARK_DEBUG "apxDestroyJvm No JVM so Done");
return FALSE;
}
}

static BOOL __apxIsJava9()
Expand Down
16 changes: 16 additions & 0 deletions src/native/windows/src/rprocess.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,22 @@ BOOL apxProcessTerminateChild(DWORD dwProcessId, BOOL dryrun)
apxLogWrite(APXLOG_MARK_DEBUG "Process ID: 0x%08X (%d) Termination failed!", treeProcessId[i], treeProcessId[i]);
}
}
} else {
/* Check if we have child processes and return FALSE if not */
HANDLE hProcess;
for (i=0; i<maxProcessId; i++) {
if (treeProcessId[i] == 0) {
break; /* Done */
}
hProcess = OpenProcess(PROCESS_ALL_ACCESS, FALSE, treeProcessId[i]);
if(hProcess != NULL) {
apxLogWrite(APXLOG_MARK_DEBUG "Process ID: 0x%08X (%d) Still here!", treeProcessId[i], treeProcessId[i]);
CloseHandle(hProcess);
CloseHandle(hProcessSnap);
return(FALSE);
}
}

}

CloseHandle(hProcessSnap);
Expand Down
9 changes: 6 additions & 3 deletions src/site/xdoc/procrun.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -530,9 +530,12 @@ will add the new value(s) to any existing value(s).
</tr>
<tr>
<td>--StopTimeout</td>
<td>No Timeout</td>
<td>Defines the timeout in seconds that procrun waits for service to
exit gracefully.</td>
<td>60 seconds</td>
<td>Defines the timeout in seconds that procrun waits for service
to receive the stop command and exit gracefully.
Note that after twice the timeout and 15 seconds the service
processes will be killed by procrun. Negative values are
replaced by the registry value of WaitToKillServiceTimeout.</td>
</tr>
<tr>
<td>--LogPath</td>
Expand Down