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

BundleContextExtension does not check for null #302

Closed
laeubi opened this issue Apr 6, 2021 · 29 comments · Fixed by #500
Closed

BundleContextExtension does not check for null #302

laeubi opened this issue Apr 6, 2021 · 29 comments · Fixed by #500

Comments

@laeubi
Copy link

laeubi commented Apr 6, 2021

If the bundles are not started that are used to acquire the bundle-context then null is used internally leading to NPE in BundleContextExtension.

@stbischof
Copy link
Contributor

stbischof commented Apr 6, 2021

How do you install the bundle?

  • Using BundleContext
  • BundleInstaller
  • injectBundle Annotation

With Junit 4 or 5

If you have an repo with an example it would help me to find the problem.

@laeubi
Copy link
Author

laeubi commented Apr 6, 2021

I have not yet tried junit4 but it might be similar there. from my setup I would assume that the problem arises if:

  1. the junit5 bundles are resolved but not started
  2. the bundle containing the test is resolved but not started

org.osgi.test.junit5.context.BundleContextExtension.getBundleContext(ExtensionContext) uses the testclass to find the bundlecontext for the test class first, if that is not present it uses org.osgi.test.junit5.context.BundleContextExtension.getParentBundleContext(ExtensionContext)

If both of them are null, a null context is passed.

@stbischof
Copy link
Contributor

Could you please give me a repo /example where i can reproduce it?

The position in code is clearer but i am not able to reproduce it.

public static BundleContext getBundleContext(ExtensionContext extensionContext) {
BundleContext bundleContext = getStore(extensionContext)
.getOrComputeIfAbsent(BUNDLE_CONTEXT_KEY,
key -> new CloseableResourceBundleContext(getParentBundleContext(extensionContext)),
CloseableResourceBundleContext.class)
.get();
return bundleContext;
}
private static BundleContext getParentBundleContext(ExtensionContext extensionContext) {
BundleContext parentContext = extensionContext.getParent()
.filter(context -> context.getTestClass()
.isPresent())
.map(BundleContextExtension::getBundleContext)
.orElseGet(() -> FrameworkUtil.getBundle(extensionContext.getRequiredTestClass())
.getBundleContext());
return parentContext;
}

@rotty3000
Copy link
Member

rotty3000 commented Apr 6, 2021 via email

@bjhargrave
Copy link
Member

I am going to close this since no reproduction was made available.

@laeubi
Copy link
Author

laeubi commented Mar 28, 2022

I am going to close this since no reproduction was made available.

It is rather obvious that there could be a NPE but its hard to reproduce since only BND setup is "supported" but just for the sake of reproduction simply create a plain surefire Junit5 test and have the following test-class:

@ExtendWith(BundleContextExtension.class)
public class BasicTest {

	@InjectBundleContext
	BundleContext bc;

	@Test
	@DisplayName("Test BundleContext injection")
	void myTest() {
		System.out.println("The injected bc is " + bc);
	}
}

Just note: running it with plain surefire will always fail as it is not running inside an OSGi container! But it should not fail with an NPE but something more descriptive e.g. neither test-bundle nor parent context could be adapted to a Bundle Context, are all bundles resolved and started?

kriegfrj added a commit to kriegfrj/osgi-test that referenced this issue Mar 28, 2022
Fixes osgi#302.

Signed-off-by: Fr Jeremy Krieg <fr.jkrieg@greekwelfaresa.org.au>
@rotty3000
Copy link
Member

rotty3000 commented Mar 28, 2022 via email

@laeubi
Copy link
Author

laeubi commented Mar 28, 2022

Its jsut the easiest way to get the NPE it also happens when the test-bundle is not started (as then there is not BundleContext).

@kriegfrj
Copy link
Contributor

You would have to jump through hoops to get a the test class from the bundle to be running while the bundle it belongs to is stopped. Mind you, if you can come up with a simple way to trigger this I would be greatly appreciated, because the patch I put together for this feature in #500 doesn't have a regression test (precisely because of the hoops I thought I would need to jump through) and I would like it to have a proper regression test.

@laeubi
Copy link
Author

laeubi commented Mar 28, 2022

if you can come up with a simple way to trigger this I would be greatly appreciated

Sure, I'm currently trying to setup a simple standalone project using osgi-test but it seems it is not even trivial to have a minimal project without a lot of config and dependencies so it seems quite hard to use osgi-test without full bnd workflow.

@rotty3000
Copy link
Member

I guess we could work on the docs first before trying to fix anything here because I feel we have more than enough proof that things work the way we intended, 'cept it's hard for beginners. Some of the assumptions (there are a few) should certainly be clarified.

@kriegfrj
Copy link
Contributor

if you can come up with a simple way to trigger this I would be greatly appreciated

Sure, I'm currently trying to setup a simple standalone project using osgi-test but it seems it is not even trivial to have a minimal project without a lot of config and dependencies so it seems quite hard to use osgi-test without full bnd workflow.

That's because osgi-test is designed to be run inside a running OSGi framework, and it's hard to launch a running OSGi framework from within Maven unless you use the full bnd workflow. Possible exceptions to this are the AssertJ bundles and the commons bundle.

If you're not using Bnd to launch your OSGi framework, how are you launching one? Are you launching one? If you're not launching one there's probably not much we can do to help, but if you are launching one using some other workflow it's possible that we might be able to accommodate it.

@kriegfrj
Copy link
Contributor

We've merged a fix now that will provide a more informative error if the injected BundleContext is null, including suggesting checking that they are running the test inside an OSGi framework.

@laeubi
Copy link
Author

laeubi commented Mar 28, 2022

Most of the time I'm using PDE/Tycho to run tests where I hit this problem here. So OSGi running but not the test bundle and as osgi-test uses FrameworkUtil.getBundle(extensionContext.getRequiredTestClass()) and this bundle does not has a bundle context at that time.

So now I tried to create a standalone maven example by using the various bnd-* plugins but this leads to that I need to provide various BND config files where it is hard to guess what is required and whats optional.

@laeubi
Copy link
Author

laeubi commented Mar 28, 2022

I guess we could work on the docs first before trying to fix anything here because I feel we have more than enough proof that things work the way we intended, 'cept it's hard for beginners.

I now uploaded my current state here : https://github.com/ops4j/org.ops4j.pax.jpa/tree/osgi-test-setup/pax-jpa-itest

if I run mvn clean verify it fails with

[INFO] Resolving /home/christoph/git/org.ops4j.pax.jpa/pax-jpa-itest/test.bndrun:
[INFO] Bnd inferred -runrequires: osgi.identity;filter:='(osgi.identity=pax-jpa-itest)'
[INFO] Bnd inferred -runee: JavaSE-11
[ERROR] Resolution failed. Capabilities satisfying the following requirements could not be found:
[<>]
⇒ osgi.identity: (osgi.identity=pax-jpa-itest)

enabling debug output does not reveal anything, and I can see that pax-jpa-itest-1.0.0-SNAPSHOT-tests.jar is created, it has a manifest that looks ok so I'm a bit lost here what could be wrong :-\

@laeubi
Copy link
Author

laeubi commented Mar 29, 2022

You would have to jump through hoops to get a the test class from the bundle to be running while the bundle it belongs to is stopped. Mind you, if you can come up with a simple way to trigger this I would be greatly appreciated, because the patch I put together for this feature in #500 doesn't have a regression test (precisely because of the hoops I thought I would need to jump through) and I would like it to have a proper regression test.

I have created a PR that adds an example using PDE: #503

Beside that it would be nice to have an PDE example it could be used to reproduce the issue:

  1. Run the example as described, test will be green
    grafik
  2. Edit the MANIFEST.MF of the example.player.impl and remove the Bundle-ActivationPolicy: lazy
  3. rerun the test, ignore any warnings pde might give you
  4. Test fails with NPE
    grafik
java.lang.NullPointerException
	at org.osgi.test.common.context.CloseableBundleContext.registerService(CloseableBundleContext.java:319)
	at java.base/java.lang.invoke.MethodHandle.invokeWithArguments(MethodHandle.java:710)
	at org.osgi.test.common.context.CloseableBundleContext.lambda$invoker$3(CloseableBundleContext.java:123)
	at org.osgi.test.common.context.CloseableBundleContext.invoke(CloseableBundleContext.java:150)
	at com.sun.proxy.$Proxy18.registerService(Unknown Source)
	at org.osgi.test.example.player.test.PlayerTest.beforeAll(PlayerTest.java:63)
	at java.base/jdk.internal.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	[...]
org.junit.jupiter.engine.execution.ExtensionValuesStore.closeAllStoredCloseableValues(ExtensionValuesStore.java:68)
		at org.junit.jupiter.engine.descriptor.AbstractExtensionContext.close(AbstractExtensionContext.java:77)
		at org.junit.jupiter.engine.execution.JupiterEngineExecutionContext.close(JupiterEngineExecutionContext.java:53)
		at org.junit.jupiter.engine.descriptor.JupiterTestDescriptor.cleanUp(JupiterTestDescriptor.java:222)
		at org.junit.jupiter.engine.descriptor.JupiterTestDescriptor.cleanUp(JupiterTestDescriptor.java:57)
		at org.junit.platform.engine.support.hierarchical.NodeTestTask.lambda$cleanUp$10(NodeTestTask.java:167)
		at org.junit.platform.engine.support.hierarchical.ThrowableCollector.execute(ThrowableCollector.java:73)
		at org.junit.platform.engine.support.hierarchical.NodeTestTask.cleanUp(NodeTestTask.java:167)
		at org.junit.platform.engine.support.hierarchical.NodeTestTask.execute(NodeTestTask.java:98)
		... 42 more

This is because in the second case the bundle under test is not started automatically, and thus do not has a BundleContext. This could be useful if for example one wants to test that a service is not available or other bundles are not activated unless another bundle is started and so on. e.g DS components are only made available for a lazy or started bundle.

@bjhargrave
Copy link
Member

This is because in the second case the bundle under test is not started automatically

Perhaps this is PDE issue? The Bnd launcher used by bnd-testing-maven-plugin start all installed bundles. You will need to get PDE to start all the bundles in the test.

@rotty3000
Copy link
Member

rotty3000 commented Mar 29, 2022

OR while using the bnd launcher, even if the bundles are lazy init, you can ask bnd to ignore that:

-runoptions: eager # bnd launcher way of ignoring the lazy init of bundles

@laeubi
Copy link
Author

laeubi commented Mar 29, 2022

This is because in the second case the bundle under test is not started automatically

Perhaps this is PDE issue? The Bnd launcher used by bnd-testing-maven-plugin start all installed bundles. You will need to get PDE to start all the bundles in the test.

No this is not a PDE issue, you can control if bundles are started or not, as you can control it in any OSGi framework, starting a bundle is not a requirement and should be simply supported. One might argue that I then can only use a "static" BundleContext (e.g. from system bundle) but should not blame the launcher for a perfectly valid OSGi state.

If osgi-test requires that the test-probe is started, it should check this and do it but does not impose its own limitations to the user.

@rotty3000
Copy link
Member

Is this a question of testing the lifecycle of bundles? If so osgi-test is semi-opinionated about testing the lifecycle of bundles. We've discussed this in the past that the idea should be that a bundle you want to test lifecycle for should not be installed as part of the initial resolved runtime, but should be installed by test code. This is why we have

@InjectBundleInstaller
BundleInstaller bundleInstaller;

The reason for this is that there are too many risks in changing the state of the bundles which might take down the entire test execution.

@laeubi
Copy link
Author

laeubi commented Mar 29, 2022

@rotty3000 for sure the BundleContextExtension might require that the test-probe is started.

Then I see some options then:

  1. it is documented somewhere and if I use the BundleContextExtension it implicitly starts the bundle if not already started
  2. it is documented somewhere and if I use the BundleContextExtension it fails the test in a controlled way with a meaningful message
  3. It might simply use the system-bundle context

Bit in no case I expect it to just fail will an cryptic internal null pointer exception leaving the user without any clue whats going on.

@rotty3000
Copy link
Member

wait! Is it merely that your resolved bundles have lazy init? Lazi init is one of those OSGi rabbit holes that I think most OSGi experts would no longer consider as a best practice.

I'm going to go out on a limb and say that modern OSGi use cases assume that lazy init is never used. I don't know if there is an official statement on that but maybe there should be a deprecation statement or something about it.

So, try adding this to your bndrun:

-runoptions: eager

OSGi test assumes all bundles are started, and if they are lazy init that some other code will init it somehow. Or use the flag above to make the launcher ignore the lazy init mode (and start bundles with eager init.)

@bjhargrave
Copy link
Member

Lazi init is one of those OSGi rabbit holes that I think most OSGi experts would no longer consider as a best practice.

Lazy activation shouldn't be a real problem IF the bundle is actually started (not in RESOLVED state).

@bjhargrave
Copy link
Member

  • it is documented somewhere and if I use the BundleContextExtension it implicitly starts the bundle if not already started

This is not a good idea. It is not the purpose of the extension to start the bundle the test code is in. It needs to be started before the test runner calls the test.

  • it is documented somewhere and if I use the BundleContextExtension it fails the test in a controlled way with a meaningful message

There was a recent PR to improve the error message. #500

  • It might simply use the system-bundle context

System bundle is wrong since it does not properly allow the function of service factories to properly work since the system bundle != bundle with test code.

@laeubi
Copy link
Author

laeubi commented Mar 29, 2022

wait! Is it merely that your resolved bundles have lazy init? Lazi init is one of those OSGi rabbit holes that I think most OSGi experts would no longer consider as a best practice.

Whole Eclipse is based on the laza-init, I won't use it but have to catch up with reality, and the question here was not if it is bad or something but how to reproduce the NPE ...

So, try adding this to your bndrun:

simpli starting all bundles don't seem better, but as above, its actually not part of the problem, I just wanted to report a programming error here (null reference not checked) and actually don't consider it turning into a discussion about BND vs PDE or lazy versus eager start and so on :-)

There was a recent PR to improve the error message. #500

yep and it was asked for a reproducer, so I provided one (executable at least in Eclipse IDE), I'm sure it can be turned in a BND test-case that is executable in the CI or something, I can just proof that the issue can actually happen so we are at least at stage 5/6 of debugging....

@bjhargrave
Copy link
Member

I made #504 to further improve reporting around no available BundleContext.

@bjhargrave
Copy link
Member

bjhargrave commented Mar 29, 2022

2. Edit the MANIFEST.MF of the example.player.impl and remove the Bundle-ActivationPolicy: lazy

It is my experience that the Eclipse configuration does not start bundles in the target unless they have Bundle-ActivationPolicy: lazy in the manifest. This is some bizarre Eclipse-ism where they are afraid to start bundles that do not have lazy activation.

See https://bugs.eclipse.org/bugs/show_bug.cgi?id=177641

@kriegfrj
Copy link
Contributor

  1. Edit the MANIFEST.MF of the example.player.impl and remove the Bundle-ActivationPolicy: lazy

It is my experience that the Eclipse configuration does not start bundles in the target unless they have Bundle-ActivationPolicy: lazy in the manifest. This is some bizarre Eclipse-ism where they are afraid to start bundles that do not have lazy activation.

It is my experience from my various attempts at getting integration tests working that this is probably because many Eclipse bundles will fail activation if the Workbench is not running. Using eager activation means it is likely that the bundle will try to activate before the Workbench is up and running (and hence fail),

@laeubi
Copy link
Author

laeubi commented Mar 30, 2022

I made #504 to further improve reporting around no available BundleContext.

This looks good, thanks for improving this, I think this will help people a lot.

It is my experience that the Eclipse configuration does not start bundles in the target unless they have Bundle-ActivationPolicy: lazy in the manifest.

You can choose in eclipse:

  1. to auto-start everything by default (default for OSGi Runconfigurations)
  2. not to auto-start everything by default (default for Product Launch Configurations)
  3. and you can choose whether or not to star individual bundle

This is some bizarre Eclipse-ism where they are afraid to start bundles that do not have lazy activation.

Simply starting everything is equally bizarre (personal opinion) but most of the time people just want it to work and don't care about carefully crafting their configurations.

For eclipse, this has also historic reasons to speedup the start-up times of the IDE where for example views that are not visible at all (and their bundles) are not started by default, similar to how DS behaves if the service is never requested and you don't force activate the service.

It is my experience from my various attempts at getting integration tests working that this is probably because many Eclipse bundles will fail activation if the Workbench is not running.

That's a nasty problem mostly cause by bad design, but is only part of the story, another one I have described above, and another is that there is no (user friendly) way to control the start-level in an Eclipse-IDE if you dynamically install software from different sources, so most developers need to choose something that fits eclipse (only start what is required) but somehow activate their bundle, that's why Bundle-ActivationPolicy: lazy is often used for eclipse plugins as it starts the bundle on first use (e.g. a View declared in the xml is started or DS component actually used).

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 a pull request may close this issue.

5 participants