SESSION_CACHE_DURATION no longer supports a "never expire" value

[dehall]

In previous releases (up to 1.0.52 at least) the env var SESSION_CACHE_DURATION, which specifies how long sessions will remain in the cache before they are purged due to inactivity, could be set to a negative value for "sessions never expire" or 0 for "sessions always expire". (Per the notes on the PassiveExpiringMap used https://commons.apache.org/proper/commons-collections/apidocs/org/apache/commons/collections4/map/PassiveExpiringMap.html#%3Cinit%3E(long) )

Testing on the 1.0.55 docker image, using a negative value causes an error at startup:

Starting instance in 0.0.0.0:3500
Exception in thread "main" org.koin.core.error.InstanceCreationException: Could not create instance for [Singleton:'controller.validation.ValidationServiceFactory']
  at org.koin.core.instance.InstanceFactory.create(InstanceFactory.kt:61)
[...]
  at io.ktor.server.jetty.JettyApplicationEngine.start(JettyApplicationEngine.kt:24)
  at ServerKt.startServer(Server.kt:81)
  at ServerKt.main(Server.kt:52)
Caused by: java.lang.IllegalArgumentException: duration cannot be negative: -1 MINUTES
  at com.google.common.base.Preconditions.checkArgument(Preconditions.java:386)
  at com.google.common.cache.CacheBuilder.expireAfterAccess(CacheBuilder.java:823)
  at controller.validation.GuavaSessionCacheAdapter.<init>(GuavaSessionCacheAdapter.kt:11)
  at controller.validation.ValidationServiceFactoryImpl.createValidationServiceInstance(ValidationServiceFactoryImpl.kt:31)
  at controller.validation.ValidationServiceFactoryImpl.<init>(ValidationServiceFactoryImpl.kt:22)
  at api.ApiInjection$koinBeans$1$1.invoke(ApiInjection.kt:20)
  at api.ApiInjection$koinBeans$1$1.invoke(ApiInjection.kt:18)
  at org.koin.core.instance.InstanceFactory.create(InstanceFactory.kt:54)
  ... 35 more

and using 0 (which per the Guava docs seems like it should be the new "never expire" https://guava.dev/releases/19.0/api/docs/com/google/common/cache/CacheBuilder.html#expireAfterAccess(long,%20java.util.concurrent.TimeUnit) ) the server starts but trying to create a session fails:

Cache size: 0
Cached new session. Cache size = 0
2024-09-10 13:10:46.532 [ktor-jetty-3500-1] ERROR ktor.application - null
java.lang.NullPointerException: null
   at org.hl7.fhir.validation.cli.services.ValidationService.validateSources(ValidationService.java:118)
   at controller.validation.ValidationControllerImpl.validateRequest(ValidationControllerImpl.kt:16)
   at controller.validation.ValidationModuleKt$validationModule$1.invokeSuspend(ValidationModule.kt:43)
   at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
   at io.ktor.util.pipeline.SuspendFunctionGun.resumeRootWith(SuspendFunctionGun.kt:138)
   at io.ktor.util.pipeline.SuspendFunctionGun.loop(SuspendFunctionGun.kt:112)
   at io.ktor.util.pipeline.SuspendFunctionGun.access$loop(SuspendFunctionGun.kt:14)
   at io.ktor.util.pipeline.SuspendFunctionGun$continuation$1.resumeWith(SuspendFunctionGun.kt:62)
   at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:46)
   at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
   at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source)
   at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)
   at java.base/java.lang.Thread.run(Unknown Source)
2024-09-10 13:10:46.548 [ktor-jetty-3500-1] ERROR ktor.application - 500 Internal Server Error: POST - /validate
java.lang.NullPointerException: Parameter specified as non-null is null: method io.ktor.util.pipeline.PipelineContextKt.pipelineContextFor, parameter subject
   at io.ktor.util.pipeline.PipelineContextKt.pipelineContextFor(PipelineContext.kt)
   at io.ktor.util.pipeline.Pipeline.createContext(Pipeline.kt:266)
   at io.ktor.util.pipeline.Pipeline.execute(Pipeline.kt:77)
   at controller.validation.ValidationModuleKt$validationModule$1.invokeSuspend(ValidationModule.kt:140)
   at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
   at io.ktor.util.pipeline.SuspendFunctionGun.resumeRootWith(SuspendFunctionGun.kt:138)
   at io.ktor.util.pipeline.SuspendFunctionGun.loop(SuspendFunctionGun.kt:112)
   at io.ktor.util.pipeline.SuspendFunctionGun.access$loop(SuspendFunctionGun.kt:14)
   at io.ktor.util.pipeline.SuspendFunctionGun$continuation$1.resumeWith(SuspendFunctionGun.kt:62)
   at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:46)
   at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
   at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(Unknown Source)
   at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(Unknown Source)
   at java.base/java.lang.Thread.run(Unknown Source)

It's not a huge deal to specify a large value instead to mean "never expire" but all of the Inferno test kits that use this validator-wrapper are currently set on -1 so it's a lot of repos to update . It would be great if we could keep the semantics of this env var consistent.

[dehall]

Hi @dotasek - sorry for the ping but we were hoping to get your thoughts on this. Inferno is trying to stay up-to-date with the validator wrapper so that results in a test kit match what a user might get on validator.fhir.org, but this unintended change has us on pause for the moment.

At minimum for us we we could add a one-liner right after this

org.hl7.fhir.validator-wrapper/src/jvmMain/kotlin/controller/validation/ValidationServiceFactoryImpl.kt

Line 27 in 29c4ce6

val sessionCacheDuration = System.getenv("SESSION_CACHE_DURATION")?.toLong() ?: SESSION_DEFAULT_DURATION;

something like:

if (sessionCacheDuration == -1) sessionCacheDuration = Long.MAX_VALUE

But that would only address the -1 case, not the 0 case. It may be simple enough to restore the 0 case as well but I haven't had time to explore. (Frankly I'm not sure the 0 case even makes sense in a tool like this but I documented it as an option in all our test kits)

The other option I see would be to make the session cache implementation choice based on another env var - by default it could use the new Guava cache but we could set it back to the PassiveExpiringSessionCache if needed.

Do you have a preference? Or any other thoughts on this

[dotasek]

@dehall SESSION_CACHE_DURATION used to be bound to PassiveExpiringMap from apache commons, which is what specified those values of -1 and 0. I don't think I realized while making the change that there were special values, and I just focused on the duration as a time value.

I was honestly hoping I could let PassiveExpiringMap die after starting to use Guava, but I guess this is not to be.

I think your suggestion of making the session cache implementation based on env var is a good one. I can implement it, but I will be out until Oct 15.

SESSION_CACHE_IMPLEMENTATION seems appropriate.
GuavaSessionCacheAdapter and PassiveExpiringSessionCache would be the supported values
GuavaSessionCacheAdapter would be the default.

From now on, those ENV variables will be treated like API, but I think we need a better solution for configuring session caches in the future.

[dehall]

Sounds good, thanks! If you really want to get rid of PassiveExpiringMap altogether, I can spend a little more time looking into how to get an "always expire" value in the Guava cache as well. If nothing stands out though I'll send a PR for this new env var. (probably Monday)

[dehall]

Upon closer inspection, turns out the 0 setting didn't work on previous versions either. The issue is this sequence in org.hl7.fhir.validation.cli.services.ValidationService

String sessionId = this.initializeValidator(request.getCliContext(), (String)null, timeTracker, request.sessionId);
ValidationEngine validationEngine = this.sessionCache.fetchSessionValidatorEngine(sessionId);

The session is created in initializeValidator and the session ID is returned, but it wasn't actually cached so the lookup on the next line returns null. That could be refactored, but it's all in the core validator code. Since we didn't actually use that setting (nor did anybody else presumably since it didn't work) I don't think it's worth pushing to support it.

So the only remaining aspect is how can we re-enable a "never expire" setting. Since you want to get rid of the PassiveExpiringMap I'll submit a one-liner PR to translate negative numbers into large numbers. Edit: I submitted both approaches so you can choose the one you prefer

[dehall]

Closing this as resolved per #191 and #192