groovy icon indicating copy to clipboard operation
groovy copied to clipboard

GROOVY-11578: provide default `FastStringService` in case of service loader failure

Open royteeuwen opened this issue 1 month ago • 7 comments

royteeuwen avatar Nov 07 '25 09:11 royteeuwen

@paulk-asert could we already go for this workaround for the linked ticket, it should not break something and will at least fix the groovy-json bundle which is the only one specifically with this kind of logic where it searches for a service loader or else returns null

royteeuwen avatar Nov 07 '25 09:11 royteeuwen

Codecov Report

:x: Patch coverage is 0% with 1 line in your changes missing coverage. Please review. :white_check_mark: Project coverage is 67.0074%. Comparing base (d0540bc) to head (ac2ce2a). :warning: Report is 69 commits behind head on master.

Files with missing lines Patch % Lines
...g/apache/groovy/json/internal/FastStringUtils.java 0.0000% 0 Missing and 1 partial :warning:
Additional details and impacted files

Impacted file tree graph

@@                Coverage Diff                 @@
##               master      #2330        +/-   ##
==================================================
+ Coverage     67.0045%   67.0074%   +0.0028%     
- Complexity      29330      29357        +27     
==================================================
  Files            1382       1382                
  Lines          116613     116623        +10     
  Branches        20432      20445        +13     
==================================================
+ Hits            78136      78146        +10     
- Misses          32038      32041         +3     
+ Partials         6439       6436         -3     
Files with missing lines Coverage Δ
...g/apache/groovy/json/internal/FastStringUtils.java 55.5556% <0.0000%> (-5.5555%) :arrow_down:

... and 25 files with indirect coverage changes

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov-commenter avatar Nov 07 '25 10:11 codecov-commenter

@eric-milles would you be able to review this :?

royteeuwen avatar Nov 25 '25 08:11 royteeuwen

@eric-milles can you recheck with my smaller list of changes :)?

royteeuwen avatar Dec 09 '25 09:12 royteeuwen

You can probably remove the null check on lines 55-57. Is there any hope for a test case?

eric-milles avatar Dec 09 '25 13:12 eric-milles

If the DefaultFastStringService comes from the groovy-json module, is it supposed to be available via service loading? Sorry, I'm a bit behind in the discussion.

eric-milles avatar Dec 09 '25 13:12 eric-milles

@eric-milles yes, it should become available through Service Loading but there are two issues:

  • Now that groovy-json is set as a Fragment, it does not work anymore, only Bundles in OSGi will be loaded through service loading, hence the change of removing the Fragment-Host property that was initially part of this PR
  • There can be a race condition, where the Bundle is still starting up and registering it's service loaders (which happens sort of async), but the groovy-json class FastStringUtils is already called, meaning that it doesn't find the service loader at that moment. So the code how it's written now is never going to work in that case because it only allows you to get the string service instance once. I hope this will be resolved in https://github.com/apache/felix-dev/pull/455 and https://github.com/osgi/osgi/pull/882

I will check if I find some time to add test cases :) I expect it will be a simple test hehe.

royteeuwen avatar Dec 09 '25 19:12 royteeuwen

@eric-milles test case added :)

royteeuwen avatar Dec 16 '25 11:12 royteeuwen