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.

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

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

Merged
merged 3 commits into from
Jan 22, 2019

Conversation

keithharp
Copy link
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
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
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
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
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
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