sksamuel / aedile

Kotlin Wrapper for Caffeine
Apache License 2.0
164 stars 15 forks source link

Added new `useCallingContext` configuration boolean #12

Closed JordanBerginSkedulo closed 11 months ago

JordanBerginSkedulo commented 1 year ago

The useCallingContext configuration (defaulted to false) causes any functions that provide a computer function at the call site to add the caller's coroutineContext to the scope used to perform the computation.

This change means contextual information such as ThreadContextElements to not be lost when those computations are run.

This applies to both Cache and LoadingCache compute functions that are provided at the call site only.

In addition to this change the following was added:

Fixes #11

sksamuel commented 1 year ago

I think this looks good. We could also not append calling context to the parent scope, but just create a new scope using the calling context. That might give users complete control over the scope at every compute call?

JordanBerginSkedulo commented 1 year ago

Awesome, I should find some time this week to feel out that solution. My only question(s) if you get time to answer it would be what is the desired behaviour when useCallingContext is used with respect to Configuration::scope and Configuration::dispatcher? Things I am unsure on:

In the interest of not blowing up the constructor parameters of the Cache and LoadingCache even further by having to pass in the Dispatcher, or having to pass in the entire configuration... I'm thinking the caffeineBuilder function could create a scope factory suspend () -> CoroutineScope that encapsulates all the complexity of the Configuration class, and we simply invoke it every time we want a scope in the Cache or LoadingCache classes? This would work with useCallingContext because it's a suspend lambda, so should have access to the current context, too.

I think it might be better explained with some code samples, I'll try to provide something this week!

sksamuel commented 1 year ago
  • If Configuration::scope and Configuration:useCallingContext is set, do we use that scope and copy it with the calling context each time?

Yes, that's what the scope + context does.

  • Configuration::dispatcher and Configuration::useCallingContext is set, do we create a new scope each time with that dispatcher?

The dispatcher is used when creating the scope and then scope + context like before.

JordanBerginSkedulo commented 1 year ago

Apologies on delay to get back to this. I think I'm tripping over myself now understanding what the code would look like if we created a new scope just with the calling context, and how that would allow users to have more control over the scope? In my head the this would mean changing

if (c.useCallingContext) scope + coroutineContext else scope

To

if (c.useCallingContext) CoroutineScope(coroutineContext) else scope

But then would this not mean Configuration::dispatcher or Configuration::scope would then be ignored? Apologies if I'm misunderstanding what's been suggested.

JordanBerginSkedulo commented 1 year ago

I've just pushed a change which moves the conditional logic of which scope to use into the caffeine.kt file and made the Caches take in a suspend () -> CoroutineScope instead (with default argument of just using the scope property so this change should be backwards compatible).

Behaviour should remain the same but now if that logic needs to change you can just change it in caffeine.kt rather than updating every callsite that uses a scope. Hope that's OK.

sksamuel commented 1 year ago

Do we think we need both scope and scope provider ?

JordanBerginSkedulo commented 1 year ago

Do we think we need both scope and scope provider ?

I suppose if we have scopeProvider then that could live on the configuration and if people wanted to provide their own scope then they can just wrap it like { scope } then? Will have a play around with it when I next get some time!

JordanBerginSkedulo commented 11 months ago

Closing this as the issue is fixed in 1.3.0-RC1