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

#394, keep dependent when unload plugin #416

Closed
wants to merge 2 commits into from
Closed

Conversation

hank-cp
Copy link
Contributor

@hank-cp hank-cp commented Jan 7, 2021

fix #394. Not sure will this change break other place.

@@ -275,7 +275,7 @@ protected boolean unloadPlugin(String pluginId, boolean unloadDependents) {
if (unloadDependents) {
List<String> dependents = dependencyResolver.getDependents(pluginId);
while (!dependents.isEmpty()) {
String dependent = dependents.remove(0);
String dependent = dependents.get(0);
Copy link
Member

Choose a reason for hiding this comment

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

I like the change because is very light.
Did you tested? I ask you because at first glance, this change produces an infinite loop (dependents will never be empty and the while block will be executed forever).

@hank-cp
Copy link
Contributor Author

hank-cp commented Jan 7, 2021

ops...I didn't notice that and left it to unit test (all passed...)

leave it for a while, I will rewrite it and add some unit test to cover the case.

@hank-cp
Copy link
Contributor Author

hank-cp commented Jan 8, 2021

@decebals updated.

also add unit test: PluginDependencyTest.dependentStop

@@ -274,8 +274,7 @@ protected boolean unloadPlugin(String pluginId, boolean unloadDependents) {
try {
if (unloadDependents) {
List<String> dependents = dependencyResolver.getDependents(pluginId);
while (!dependents.isEmpty()) {
String dependent = dependents.remove(0);
for (String dependent : dependents) {
unloadPlugin(dependent, false);
dependents.addAll(0, dependencyResolver.getDependents(dependent));
Copy link
Member

Choose a reason for hiding this comment

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

I think that this line will produce in the end a java.util.ConcurrentModificationException.

@decebals
Copy link
Member

decebals commented Jan 8, 2021

I see something related to this issue. It's about #250.
Maybe it helps.

@decebals decebals mentioned this pull request Jan 8, 2021
@decebals
Copy link
Member

decebals commented Jan 8, 2021

Replaced by #416. Much of the work was done by by @hank-cp. Thanks!

@decebals decebals closed this Jan 8, 2021
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.

DependencyResolver lost dependent info after plugin stop
2 participants