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

fix: NPE on named provider init/shutdown #595

Merged
merged 2 commits into from
Sep 7, 2023

Conversation

liran2000
Copy link
Member

fix NPE on named provider initialization.

Problem

When initializing named provider first time, NPE is thrown.
This may cause old provider to not shutdown properly.

Solution

Add null check on isProviderRegistered.

Steps to reproduce

  1. Add dependency at pom.xml to see logs on console:
<dependency>
      <groupId>org.apache.logging.log4j</groupId>
      <artifactId>log4j-slf4j2-impl</artifactId>
      <version>2.20.0</version>
      <scope>test</scope>
</dependency>
  1. Run relevant individual unit test, e.g. dev.openfeature.sdk.EventsTest.ApiEvents.NamedProvider.Initialization.
  2. Observe console log. Without this fix, the following error appears:
java.lang.NullPointerException: null
	at java.util.concurrent.ConcurrentHashMap.containsValue(ConcurrentHashMap.java:979) ~[?:?]
	at dev.openfeature.sdk.ProviderRepository.isProviderRegistered(ProviderRepository.java:145) ~[classes/:?]
	at dev.openfeature.sdk.ProviderRepository.shutDownOld(ProviderRepository.java:138) ~[classes/:?]
	at dev.openfeature.sdk.ProviderRepository.initializeProvider(ProviderRepository.java:130) ~[classes/:?]
	at dev.openfeature.sdk.ProviderRepository.lambda$prepareAndInitializeProvider$2(ProviderRepository.java:115) ~[classes/:?]
	at java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:539) ~[?:?]
	at java.util.concurrent.FutureTask.run(FutureTask.java:264) ~[?:?]
	at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1136) ~[?:?]
	at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:635) ~[?:?]
	at java.lang.Thread.run(Thread.java:833) ~[?:?]

Signed-off-by: liran2000 <liran2000@gmail.com>
@liran2000 liran2000 requested a review from a team as a code owner September 7, 2023 11:48
@codecov
Copy link

codecov bot commented Sep 7, 2023

Codecov Report

Merging #595 (682b609) into main (a2e4894) will increase coverage by 0.12%.
The diff coverage is 66.66%.

@@             Coverage Diff              @@
##               main     #595      +/-   ##
============================================
+ Coverage     94.53%   94.65%   +0.12%     
- Complexity      357      359       +2     
============================================
  Files            32       32              
  Lines           841      842       +1     
  Branches         51       52       +1     
============================================
+ Hits            795      797       +2     
+ Misses           25       24       -1     
  Partials         21       21              
Flag Coverage Δ
unittests 94.65% <66.66%> (+0.12%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files Changed Coverage Δ
...n/java/dev/openfeature/sdk/ProviderRepository.java 96.96% <66.66%> (-1.50%) ⬇️

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

📢 Have feedback on the report? Share it here.

Copy link
Member

@toddbaert toddbaert left a comment

Choose a reason for hiding this comment

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

Nice find, thanks!

If you feel like it, you could use Optional here, but that's preference.

I will merge this by EOD.

@toddbaert toddbaert changed the title fix: fix NPE on named provider init fix: NPE on named provider init Sep 7, 2023
Signed-off-by: liran2000 <liran2000@gmail.com>
@sonarqubecloud
Copy link

sonarqubecloud bot commented Sep 7, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@toddbaert toddbaert changed the title fix: NPE on named provider init fix: NPE on named provider init/shutdown Sep 7, 2023
@toddbaert toddbaert merged commit d063bf2 into open-feature:main Sep 7, 2023
@liran2000 liran2000 deleted the feat/fix_register_npe branch September 7, 2023 20:20
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