Skip to content

Update W32Service.stopService() to be more resilient to large dwWaitH…#1058

Merged
matthiasblaesing merged 3 commits intojava-native-access:masterfrom
keithharp:master
Jan 22, 2019
Merged

Update W32Service.stopService() to be more resilient to large dwWaitH…#1058
matthiasblaesing merged 3 commits intojava-native-access:masterfrom
keithharp:master

Conversation

@keithharp
Copy link
Copy Markdown
Contributor

…int values

-- make stopService(long timeout) public so that 30 seconds is not the only valid timeout
-- never wait more than the current timeout
-- if the service stops during the last sleep, do not throw an exception
-- use the preexisting timing logic in waitForNonPendingState to sleep fractions of the total dwWaitHint instead of the entire time
-- get rid of some leftover code that no longer served a purpose

…int values

-- make stopService(long timeout) public so that 30 seconds is not the only valid timeout
-- never wait more than the current timeout
-- if the service stops during the last sleep, do not throw an exception
-- use the preexisting timing logic in waitForNonPendingState to sleep fractions of the total dwWaitHint instead of the entire time
@matthiasblaesing
Copy link
Copy Markdown
Member

Thank you Keith, the change looks sane to me, before merging this, I have one question: Was this tested? If so how?
For the change it would be great to document it in the CHANGES.md in the toplevel directory. Feel free to decide whether you consider this a feature or a bugfix, it is already clear, that the next release will be a feature release. Please see the existing entries for a format reference.

@keithharp
Copy link
Copy Markdown
Contributor Author

Pull request has been updated with the CHANGES.md update.

Testing was primarily conducted against a prorun driven service I had source to, where I could control
the dwWaitHint and how long it took to stop. I verified ( (the ones with asterisks are the improvements over the previous code)
a) varying timeout values *
b) an exception is thrown if the timeout is exceeded
c) services stopped
d) if the dwWaitHint was longer than the timeout value and the service was successfully stopped before the requested timeout value, then no exception was thrown. *
e) if the dwWaitHint was longer than needed, the routine returned faster than the previous version *
f) if the dwWaitHint was longer than the timeout value and the service was not successfully stopped before the timeout value, an exception would be thrown at the timeout value, rather than at the end of dwWaitHint *

I also verified the routine still worked against a few other random services.

throw new RuntimeException(String.format("Service stop exceeded timeout time of %d ms", timeout));
}

int dwWaitTime = Math.min(sanitizeWaitTime(status.dwWaitHint), (int)msRemainingBeforeTimeout);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This needs to be declared as a long. The user could pass in a timeout value > Integer.MAX_VALUE, so msRemainingBeforeTimeout could also be > Integer.MAX_VALUE. After the cast, the value will wrap around and you will get negative wait time.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed. Done and committed.

@matthiasblaesing
Copy link
Copy Markdown
Member

Thank you - looks good to me.

@matthiasblaesing matthiasblaesing merged commit 904a71e into java-native-access:master Jan 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants