Arrange the timeout logic and values. #238
Conversation
|
The documentation also needs to be updated to reflect all of these changes. |
| if (timeout) { | ||
| /* the SO_STOPTIMEOUT applies to the stop command and to the time service needs to stop */ | ||
| /* We also add 15 seconds for the service logic */ |
There was a problem hiding this comment.
Is that the reason for the *2 below? I assume so, but I could be wrong.
There was a problem hiding this comment.
Yes in fact the timeout is applied to the STOP command (which connect to the running service) and later to the loop that waits for the child processes to exit. A 15 seconds is added to allow procrun to communicate with windows service manager.
|
@sebbASF I did the changes you requested @markt-asf I have adjusted the doc, should I merge now? |
|
Hello @jfclere |
|
@garydgregory By hands on a windows VS2019, you may create an issue to think about automated tests... |
There was a problem hiding this comment.
Some of these changes are improvements to debug logging and could probably be made independently of this PR. That may make this PR simpler to review if it is just focused on the logic changes to shutdown.
The additional comments were welcome.
I think it would be better from a user perspective (if is possible) for the user to set the total timeout and procrun to then figure out what timeouts to use at each stage.
| if (timeout == 0) { | ||
| /* waiting for ever doesn't look OK here */ | ||
| timeout = ONE_MINUTE_SEC; | ||
| } | ||
| if (timeout) { |
There was a problem hiding this comment.
timeout cannot be zero here this this will always be true. Therefore, no need for the if statement.
| @@ -952,9 +955,16 @@ static BOOL docmdStopService(LPAPXCMDLINE lpCmdline) | |||
| if (!rv) { | |||
| /* Wait for the timeout if any */ | |||
| int timeout = SO_STOPTIMEOUT; | |||
| if (timeout == 0) { | |||
| int i; | ||
| for (i = 0; i < timeout; i++) { | ||
| for (i = 0; i < (timeout*2+TIMEFORSERVICEMANAGER); i++) { |
There was a problem hiding this 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?
| @@ -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); | |||
There was a problem hiding this comment.
Why is this hard-coded? Shouldn't it depend on the 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; |
There was a problem hiding this comment.
Again, isn't timeout now always >0 ?
| @@ -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 */ | |||
There was a problem hiding this comment.
Doesn't this wait for timeout seconds rather than waiting forever?
| DWORD count = SO_STOPTIMEOUT; | ||
| if (!count) | ||
| count = ONE_MINUTE_SEC; | ||
| do { |
There was a problem hiding this comment.
This looks like a possible 3rd wait using the stop timeout.
| @@ -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); | |||
There was a problem hiding this comment.
Why hard-coded to a minute?
Fix for https://issues.apache.org/jira/browse/DAEMON-468