markt-asf commented on code in PR #238:
URL: https://github.com/apache/commons-daemon/pull/238#discussion_r1983049640
##########
src/native/windows/apps/prunsrv/prunsrv.c:
##########
@@ -952,9 +955,16 @@ static BOOL docmdStopService(LPAPXCMDLINE lpCmdline)
if (!rv) {
/* Wait for the timeout if any */
int timeout = SO_STOPTIMEOUT;
+ if (timeout == 0) {
+ /* waiting for ever doesn't look OK here */
+ timeout = ONE_MINUTE_SEC;
+ }
if (timeout) {
Review Comment:
`timeout` cannot be zero here this this will always be `true`. Therefore, no
need for the `if` statement.
##########
src/native/windows/apps/prunsrv/prunsrv.c:
##########
@@ -952,9 +955,16 @@ static BOOL docmdStopService(LPAPXCMDLINE lpCmdline)
if (!rv) {
/* Wait for the timeout if any */
int timeout = SO_STOPTIMEOUT;
+ if (timeout == 0) {
Review Comment:
Why not `<=` here?
##########
src/native/windows/apps/prunsrv/prunsrv.c:
##########
@@ -952,9 +955,16 @@ static BOOL docmdStopService(LPAPXCMDLINE lpCmdline)
if (!rv) {
/* Wait for the timeout if any */
int timeout = SO_STOPTIMEOUT;
+ if (timeout == 0) {
+ /* waiting for ever doesn't look OK here */
+ timeout = ONE_MINUTE_SEC;
+ }
if (timeout) {
+ /* 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++) {
Review Comment:
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?
##########
src/native/windows/apps/prunsrv/prunsrv.c:
##########
@@ -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);
Review Comment:
Why hard-coded to a minute?
##########
src/native/windows/apps/prunsrv/prunsrv.c:
##########
@@ -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 */
Review Comment:
Doesn't this wait for `timeout` seconds rather than waiting forever?
##########
src/native/windows/apps/prunsrv/prunsrv.c:
##########
@@ -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 {
Review Comment:
This looks like a possible 3rd wait using the stop timeout.
##########
src/native/windows/apps/prunsrv/prunsrv.c:
##########
@@ -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);
Review Comment:
Why is this hard-coded? Shouldn't it depend on the timeout?
##########
src/native/windows/apps/prunsrv/prunsrv.c:
##########
@@ -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;
Review Comment:
Again, isn't timeout now always >0 ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscr...@commons.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
