markt-asf commented on code in PR #238:
URL: https://github.com/apache/commons-daemon/pull/238#discussion_r2081081047
##########
src/native/windows/apps/prunsrv/prunsrv.c:
##########
@@ -42,6 +42,7 @@
#define STDOUT_FILENO 1
#define STDERR_FILENO 2
#define ONE_MINUTE (60 * 1000)
+#define ONE_MINUTE_SEC 60
Review Comment:
`ONE_MINUTE_SEC` looks weird every time I look at it even when I know what
it means. Maybe
```
ONE_MINUTE_AS_MILLIS
ONE_MINUTE_AS_SEC
```
##########
src/native/windows/apps/prunsrv/prunsrv.c:
##########
@@ -258,6 +259,8 @@ static BOOL gSignalValid = TRUE;
static APXJAVA_THREADARGS gRargs;
static APXJAVA_THREADARGS gSargs;
+static DWORD stopStarted = 0; /* Not stop not started */
Review Comment:
Comment looks odd. Does it mean the time that the stop process began? A
rename might be helpful here. Maybe `stopCalledTime`?
##########
src/native/windows/apps/prunsrv/prunsrv.c:
##########
@@ -951,15 +970,34 @@ static BOOL docmdStopService(LPAPXCMDLINE lpCmdline)
NULL);
if (!rv) {
/* Wait for the timeout if any */
- int timeout = SO_STOPTIMEOUT;
- if (timeout) {
- int i;
- for (i = 0; i < timeout; i++) {
- rv = apxServiceCheckStop(hService);
- apxLogWrite(APXLOG_MARK_DEBUG "apxServiceCheck returns
%d.", rv);
- if (rv)
- break;
- }
+ int timeout = SO_STOPTIMEOUT;
+ int waited = waitedSinceStopCmd();
+ int i;
+ if (timeout <= 0) {
+ /* waiting for ever doesn't look OK here */
+ timeout = ONE_MINUTE_SEC;
+ }
+ apxLogWrite(APXLOG_MARK_DEBUG "docmdStopService Waited %d timeout
%d", waited, timeout*1000);
+ if (timeout*1000 > waited) {
+ /*
+ * it took waited to send the command, the service starts
+ * counting the timeout once it receives the STOP command
+ * that is probably after waited but that depends on the
+ * box we are using, we add 1 second just to be sure.
+ */
+ timeout = timeout*1000 + waited + 1000;
+ } else {
+ apxLogWrite(APXLOG_MARK_DEBUG "docmdStopService timeout %d too
small.", timeout*1000);
+ timeout = 1000; /* we might wait 1 s too much but we check if
we have already stopped */
+ }
+ apxLogWrite(APXLOG_MARK_DEBUG "docmdStopService estimated timeout
%d milliseconds.", timeout);
+ /* the SO_STOPTIMEOUT applies to the stop command and to the time
service needs to stop */
+ for (i = 0; i < timeout; i=i+1000) {
Review Comment:
`apxServiceCheckStop` will take at least 1000ms because of the wait. Is
there any chance it could take longer than that? Should this loop use
`waitedSinceStopCmd()` rather than `i`?
##########
src/native/windows/apps/prunsrv/prunsrv.c:
##########
@@ -951,15 +970,34 @@ static BOOL docmdStopService(LPAPXCMDLINE lpCmdline)
NULL);
if (!rv) {
/* Wait for the timeout if any */
- int timeout = SO_STOPTIMEOUT;
- if (timeout) {
- int i;
- for (i = 0; i < timeout; i++) {
- rv = apxServiceCheckStop(hService);
- apxLogWrite(APXLOG_MARK_DEBUG "apxServiceCheck returns
%d.", rv);
- if (rv)
- break;
- }
+ int timeout = SO_STOPTIMEOUT;
+ int waited = waitedSinceStopCmd();
+ int i;
+ if (timeout <= 0) {
+ /* waiting for ever doesn't look OK here */
+ timeout = ONE_MINUTE_SEC;
+ }
+ apxLogWrite(APXLOG_MARK_DEBUG "docmdStopService Waited %d timeout
%d", waited, timeout*1000);
+ if (timeout*1000 > waited) {
+ /*
+ * it took waited to send the command, the service starts
+ * counting the timeout once it receives the STOP command
+ * that is probably after waited but that depends on the
+ * box we are using, we add 1 second just to be sure.
+ */
+ timeout = timeout*1000 + waited + 1000;
+ } else {
+ apxLogWrite(APXLOG_MARK_DEBUG "docmdStopService timeout %d too
small.", timeout*1000);
+ timeout = 1000; /* we might wait 1 s too much but we check if
we have already stopped */
+ }
+ apxLogWrite(APXLOG_MARK_DEBUG "docmdStopService estimated timeout
%d milliseconds.", timeout);
+ /* the SO_STOPTIMEOUT applies to the stop command and to the time
service needs to stop */
Review Comment:
This is really useful information that should probably be in the docs. Does
it make sense to use the same timeout for both?
##########
src/native/windows/apps/prunsrv/prunsrv.c:
##########
@@ -1223,16 +1261,26 @@ static DWORD WINAPI serviceStop(LPVOID lpParameter)
BOOL wait_to_die = FALSE;
DWORD timeout = SO_STOPTIMEOUT * 1000;
DWORD dwCtrlType = (DWORD)((BYTE *)lpParameter - (BYTE *)0);
+ DWORD waited = 0;
apxLogWrite(APXLOG_MARK_INFO "Stopping service...");
+ stopStarted = GetTickCount();
+ if (dwCtrlType == SERVICE_CONTROL_SHUTDOWN)
+ timeout = MIN(timeout, apxGetMaxServiceTimeout(gPool));
+ if (!timeout) {
+ /* Use 1 minutes default */
Review Comment:
Drop the 's' from minutes.
##########
src/native/windows/apps/prunsrv/prunsrv.c:
##########
@@ -1375,11 +1418,18 @@ static DWORD WINAPI serviceStop(LPVOID lpParameter)
CloseHandle(gSignalThread);
gSignalEvent = NULL;
}
- if (wait_to_die && !timeout)
- timeout = 300 * 1000; /* Use the 5 minute default shutdown */
- if (dwCtrlType == SERVICE_CONTROL_SHUTDOWN)
- timeout = MIN(timeout, apxGetMaxServiceTimeout(gPool));
+ /* We already waited here timeout is msec */
+ waited = waitedSinceStopCmd();
+ apxLogWrite(APXLOG_MARK_DEBUG "Waited %d timeout (%d)", waited, timeout);
+ if (timeout > waited) {
+ timeout = timeout - waited;
+ } else {
+ /* something is wrong, the timeout is too small */
+ apxLogWrite(APXLOG_MARK_DEBUG "Waiting more than the specified timeout
(%d)", timeout);
+ }
Review Comment:
Include how long we have waited in this message?
##########
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:
Maybe update the comment to reflect that? "for the service to be done"
suggests "until the service has stopped" rather than "until the call to stop
the service has returned".
##########
src/native/windows/apps/prunsrv/prunsrv.c:
##########
@@ -951,15 +970,34 @@ static BOOL docmdStopService(LPAPXCMDLINE lpCmdline)
NULL);
if (!rv) {
/* Wait for the timeout if any */
- int timeout = SO_STOPTIMEOUT;
- if (timeout) {
- int i;
- for (i = 0; i < timeout; i++) {
- rv = apxServiceCheckStop(hService);
- apxLogWrite(APXLOG_MARK_DEBUG "apxServiceCheck returns
%d.", rv);
- if (rv)
- break;
- }
+ int timeout = SO_STOPTIMEOUT;
+ int waited = waitedSinceStopCmd();
+ int i;
+ if (timeout <= 0) {
+ /* waiting for ever doesn't look OK here */
+ timeout = ONE_MINUTE_SEC;
+ }
Review Comment:
If we aren't ever going to allow an infinite timeout, why let users create
one in the first place? Should we reject any attempt to create such a service?
If we go that route, we'll need to allow for existing services that do have an
infinite timeout. Should be log a warning that we are ignoring the timeout?
##########
src/native/windows/apps/prunsrv/prunsrv.c:
##########
@@ -1609,11 +1659,14 @@ 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) {
+ stopStarted = GetTickCount();
+ /* hint for shutdown time */
+ if (SO_STOPTIMEOUT) {
reportServiceStatus(SERVICE_STOP_PENDING, NO_ERROR,
SO_STOPTIMEOUT * 1000);
}
else {
- reportServiceStatus(SERVICE_STOP_PENDING, NO_ERROR, 3 * 1000);
+ /* Use 1 minutes default */
Review Comment:
s/minutes/minute/
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
