You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by GitBox <gi...@apache.org> on 2021/11/22 18:19:20 UTC
[GitHub] [sling-org-apache-sling-testing-osgi-mock] kwin opened a new pull request #14: SLING-10930 add failing test with multiple deactivate() calls on the
kwin opened a new pull request #14:
URL: https://github.com/apache/sling-org-apache-sling-testing-osgi-mock/pull/14
same component
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-osgi-mock] stefanseifert commented on pull request #14: SLING-10930 add failing test with multiple deactivate() calls on the
Posted by GitBox <gi...@apache.org>.
stefanseifert commented on pull request #14:
URL: https://github.com/apache/sling-org-apache-sling-testing-osgi-mock/pull/14#issuecomment-977825162
i've implemented a simple fix as proposal for this issue, to ensure we are not calling deactivate multiple times.
sidenote: i've seen that the getService() method used in this fix is a) ill-named and b) will lead to problems when a ServiceFactory is used (something that is probably used very rarely and i think not correctly supported currently anyways - this will be subject to another issue)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-osgi-mock] sonarcloud[bot] removed a comment on pull request #14: SLING-10930 add failing test with multiple deactivate() calls on the
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #14:
URL: https://github.com/apache/sling-org-apache-sling-testing-osgi-mock/pull/14#issuecomment-977827071
SonarCloud Quality Gate failed. 
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&metric=new_coverage&view=list) [58.3% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-osgi-mock] sonarcloud[bot] commented on pull request #14: SLING-10930 add failing test with multiple deactivate() calls on the
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #14:
URL: https://github.com/apache/sling-org-apache-sling-testing-osgi-mock/pull/14#issuecomment-977827071
SonarCloud Quality Gate failed. 
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&metric=new_coverage&view=list) [58.3% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-osgi-mock] kwin commented on a change in pull request #14: SLING-10930 add failing test with multiple deactivate() calls on the
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #14:
URL: https://github.com/apache/sling-org-apache-sling-testing-osgi-mock/pull/14#discussion_r756070321
##########
File path: core/src/main/java/org/apache/sling/testing/mock/osgi/MockBundleContext.java
##########
@@ -455,13 +455,19 @@ public File getDataFile(final String path) {
public void shutdown() {
List<MockServiceRegistration> reversedRegisteredServices = new ArrayList<>(registeredServices);
Collections.reverse(reversedRegisteredServices);
+ Set<Object> shutdownInnstances = new HashSet<>();
Review comment:
typo: shutdownInnstances -> shutdownInstances
I would prefer a name like `deactivatedComponents`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-osgi-mock] kwin merged pull request #14: SLING-10930 add failing test with multiple deactivate() calls on the
Posted by GitBox <gi...@apache.org>.
kwin merged pull request #14:
URL: https://github.com/apache/sling-org-apache-sling-testing-osgi-mock/pull/14
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-osgi-mock] sonarcloud[bot] commented on pull request #14: SLING-10930 add failing test with multiple deactivate() calls on the
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #14:
URL: https://github.com/apache/sling-org-apache-sling-testing-osgi-mock/pull/14#issuecomment-977961355
SonarCloud Quality Gate failed. 
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=CODE_SMELL) [1 Code Smell](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&metric=new_coverage&view=list) [61.5% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-testing-osgi-mock&pullRequest=14&metric=new_duplicated_lines_density&view=list)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-osgi-mock] kwin commented on a change in pull request #14: SLING-10930 add failing test with multiple deactivate() calls on the
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #14:
URL: https://github.com/apache/sling-org-apache-sling-testing-osgi-mock/pull/14#discussion_r756070517
##########
File path: core/src/main/java/org/apache/sling/testing/mock/osgi/MockBundleContext.java
##########
@@ -455,13 +455,19 @@ public File getDataFile(final String path) {
public void shutdown() {
List<MockServiceRegistration> reversedRegisteredServices = new ArrayList<>(registeredServices);
Collections.reverse(reversedRegisteredServices);
+ Set<Object> shutdownInnstances = new HashSet<>();
for (MockServiceRegistration<?> serviceRegistration : reversedRegisteredServices) {
+ Object componentInstance = serviceRegistration.getService();
+ if (shutdownInnstances.contains(componentInstance)) {
+ continue;
+ }
try {
MockOsgi.deactivate(serviceRegistration.getService(), this, serviceRegistration.getProperties());
Review comment:
use componentInstance here as first argument
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-osgi-mock] stefanseifert commented on pull request #14: SLING-10930 add failing test with multiple deactivate() calls on the
Posted by GitBox <gi...@apache.org>.
stefanseifert commented on pull request #14:
URL: https://github.com/apache/sling-org-apache-sling-testing-osgi-mock/pull/14#issuecomment-977958658
we have two different issues here:
* a) deactivate may be called twice - we have to prevent this even if we had a real DS component registry
* b) calling deactivate only for DS components
this fix in this PR fixes a). we can always improve b) later if it worries us.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-osgi-mock] kwin commented on pull request #14: SLING-10930 add failing test with multiple deactivate() calls on the
Posted by GitBox <gi...@apache.org>.
kwin commented on pull request #14:
URL: https://github.com/apache/sling-org-apache-sling-testing-osgi-mock/pull/14#issuecomment-977891648
I am still not sure about this fix, as this may non-deliberately call a method on a simple service which is coincidentally also called "deactivate()", but this is not new behaviour. Therefore I would still rather not call deactivate at all (instead of calling it unconditionally on all services). A proper fix is as I said a dedicated component registry (decoupled from the service registry).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-testing-osgi-mock] kwin commented on a change in pull request #14: SLING-10930 add failing test with multiple deactivate() calls on the
Posted by GitBox <gi...@apache.org>.
kwin commented on a change in pull request #14:
URL: https://github.com/apache/sling-org-apache-sling-testing-osgi-mock/pull/14#discussion_r756068175
##########
File path: test-services/src/main/java/org/apache/sling/testing/mock/osgi/testsvc/osgiserviceutil/activatedeactivate/Service8.java
##########
@@ -40,7 +40,8 @@ private void activate(BundleContext bundleContext) {
@Deactivate
private void deactivate(BundleContext bundleContext) {
serviceRegistration.unregister();
- //serviceRegistration = null; // this may cause a NPE with subsequent deactivate() calls
+ // explicitly set to null to ensure deactivate is never called twice
+ serviceRegistration = null;
Review comment:
This will lead to another exception (NPE instead of ISE)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org