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

Breaking: change Service.delete to borrow rather than consume self #90

Merged
merged 2 commits into from
Feb 17, 2023
Merged

Breaking: change Service.delete to borrow rather than consume self #90

merged 2 commits into from
Feb 17, 2023

Conversation

citreae535
Copy link
Contributor

@citreae535 citreae535 commented Feb 7, 2023

The underlying win32 API call merely marks the service for deletion. It can be called even if the service is not stopped and will not invalidate the caller's open handle to it. Making the function not consuming self allows the caller to decide when to close the handle. Docs and examples are updated to show the correct behavior of this API.


This change is Reviewable

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

Reviewed 7 of 7 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor

@pronebird pronebird left a comment

Choose a reason for hiding this comment

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

I think it makes sense esp. because deletion can fail so it makes no sense to consume the Service. I am not sure there was any need to go through all examples and change println! but I'll leave it up to someone else to review. I am still using println! in the old ways and not very used to {variable}.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@citreae535
Copy link
Contributor Author

Those println! are changed because this clippy lint, uninlined_format_args, shows up as warnings by default in 1.67.0. It was changed to allow in 1.67.1 but the maintainers said they "should restore this when rust-analyzer support gets better". As long as it is consistent I don't really have a strong opinion on this. Might be a good idea adding a clippy.toml if the old way is preferable.

@citreae535
Copy link
Contributor Author

This one can wait for #89 to make use of that error helper function in the example.

@citreae535
Copy link
Contributor Author

Just rebased it. Should be ready to merge.

@pronebird
Copy link
Contributor

pronebird commented Feb 15, 2023

We had a call to thread::sleep() in uninstall example to let the service stop. Do you think we should keep it to be on the safe side? Feels like there can be a race between the call to stop the service and service_manager.open_service().

@pronebird
Copy link
Contributor

@citreae535 previous PRs are already merged so you could perhaps rebase this one and see my other comment above too.

@citreae535
Copy link
Contributor Author

The uninstall example now polls for service stop and deletion with a timeout.

@pronebird pronebird merged commit ac6c4e7 into mullvad:master Feb 17, 2023
@pronebird
Copy link
Contributor

Merged. Good work & thanks for contributions! 👍

@citreae535 citreae535 deleted the change_delete branch February 17, 2023 13:28
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.

2 participants