Fix list enumeration #11851
Fix list enumeration #11851
Conversation
ghost
assigned
There was a problem hiding this comment.
Is there a reason you're enumerating from the end to the beginning here?
Also, I don't see a real need to use a for loop here. For loops are nice, but tend to create confusing and less readable code on the whole.
Perhaps an alternative solution would be to attempt to stop each service, then filter out the stopped services with one of the List filter methods after the foreach loop is finished?
There was a problem hiding this comment.
Is there a reason you're enumerating from the end to the beginning here?
Enumerating from the end tends to move fewer items.
Also, I don't see a real need to use a for loop here. For loops are nice, but tend to create confusing and less readable code on the whole.
Sounds good. Updated to use RemoveAll (and TrueForAll for another method BTW).
Perhaps an alternative solution would be to attempt to stop each service
The services have been requested to stop before this filter method.
vexx32
left a comment
vexx32
left a comment
There was a problem hiding this comment.
Looks good to me overall! One minor thing you might want to double check just to be sure it won't cause any issues, but it doesn't seem like it should break anything as far as I can see.
Thanks for sorting this out! 😊
If there's a scenario you know was previously broken, it might be a good idea to add a test to ensure that scenario is completely fixed with this change and does not regress later on. 🙂
| /// False if not all dependent services are stopped | ||
| /// </returns> | ||
| private bool HaveAllDependentServicesStopped(ICollection<ServiceController> dependentServices) | ||
| private bool HaveAllDependentServicesStopped(ServiceController[] dependentServices) |
There was a problem hiding this comment.
I'm not sure what the possible impact of changing the method signature here would be, but given it's a private method anyway, it's probably safe to do as long as the code calling the method is still OK to call it with this change.
There was a problem hiding this comment.
This method is only called from one place, hence I believe it's separated to provide better readability.
It makes sense to cover these code paths. The tests will require a custom service, which refuses to accept the stop command during testing but accepts it when uninstalled... I don't want to tackle that at this moment. :( |
|
That does sound rather complicated, indeed. The fix seems logically sound, though, so unless there's a more straightforward route to testing it, that might have to come later. 🙂 |
|
@NextTurn Please recover PR template to original and add a reference to an issue with a discussion. |
Opened #11858 |
iSazonov
left a comment
iSazonov
left a comment
There was a problem hiding this comment.
LGTM with one minor comment.
| services.Remove(service); | ||
| } | ||
| } | ||
| services.RemoveAll(service => |
There was a problem hiding this comment.
Please add a comment why we have to use LINQ.
There was a problem hiding this comment.
This is not LINQ. It's a method of List<T>.
There was a problem hiding this comment.
It is just better. Please add a comment.
iSazonov
added
the
CL-General
| internal void RemoveNotStoppedServices(List<ServiceController> services) | ||
| { | ||
| // You shall not modify a collection during enumeration. | ||
| // https://stackoverflow.com/questions/1582285/how-to-remove-elements-from-a-generic-list-while-iterating-over-it |
There was a problem hiding this comment.
No need to reference StackOverflow. A simple comment is enough that we cannot use an enumerator.
|
@TravisEz13 @daxian-dbw The PR is ready to merge. |
|
@NextTurn Thank you for your contribution! |
|
🎉 Handy links: |
5 participants
PR Summary
Fixes list enumeration and removal.
PR Context
A collection shall not be modified during enumeration.
PR Checklist
.h,.cpp,.cs,.ps1and.psm1files have the correct copyright header