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

Investigate Scoped Values #108

Open
shakuzen opened this issue Jun 5, 2023 · 7 comments
Open

Investigate Scoped Values #108

shakuzen opened this issue Jun 5, 2023 · 7 comments
Labels
enhancement A general enhancement
Milestone

Comments

@shakuzen
Copy link
Member

shakuzen commented Jun 5, 2023

Scoped Values (Preview) is proposed to target JDK 21: https://openjdk.org/jeps/446

I haven't put a ton of thought into what an integration with this might look like in context-propagation, but it seems worth investigating as a lightweight alternative to propagation via ThreadLocal. It would be good if we can give any feedback about Scoped Values before it exits preview.

@shakuzen shakuzen added the enhancement A general enhancement label Jun 5, 2023
@shakuzen shakuzen added this to the 1.1.0-M3 milestone Jun 5, 2023
@jonatan-ivanov jonatan-ivanov modified the milestones: 1.1.0-M3, 1.0.x, 1.1.x Jul 10, 2023

This comment was marked as outdated.

@ppkarwasz
Copy link

@shakuzen,

Do you already have a strategy to deal with ScopedValue?

Since ScopedValue has no "setter" method, only a Runnable "wrapper" method (runWhere), a naïve approach would be to create an new ScopedValueAccessor interface:

public interface ScopedValueAccessor<V> {

    /**
     * @see ThreadLocalAccessor#key()
     */
    Object key();

    /**
     * @see ThreadLocalAccessor#getValue()
     */
    @Nullable
    V getValue();

   /**
    * Returns a wrapped runnable that:
    * <ol>
    *   <li>Adds value to the context of the current thread,</li>
    *   <li>Runs {@code runnable},</li>
    *   <li>Cleans up the context of the current thread.
    * </ol>
    */
    Runnable wrap(Runnable runnable, V value);

  // Similar methods for `Callable<R>` and `Supplier<R>`.
  ...
}

and use ScopedValueAccessor#wrap to set up the value of a ScopedValue on a new thread.

This approach, however seems very inefficient. The ScopedValue Javadoc states:

Scoped values are designed to be used in fairly small numbers.

This might require an inversion of the approach used by context-propagation: instead of creating snapshots from multiple ThreadLocals used by foreign APIs, you could provide a SPI that allows foreign APIs to store and retrieve values in a map-like structure provided by context-propagation.

Concrete example

The current way to propagate Log4j's ThreadContext between threads is to create a ThreadLocalAccessor that calls ThreadContext#getImmutableContext to create a snapshot and ThreadContext#putAll to propagate a snapshot to a new thread.

However, there is another approach possible. Users can create a ThreadContextMap (the service behind most ThreadContext methods) implementation that stores its data in a structure provided by context-propagation and configure Log4j API to use this implementation.

What do you think?

@shakuzen
Copy link
Member Author

Thanks for taking a look at this. Unfortunately, I haven't had the bandwidth to give this the attention it deserves yet. I have not looked into it beyond an initial read of the JEP. When I have time, I'd love to take a closer look at it and your proposal here. In the meantime, I'm seeing if anyone else from the team has bandwidth or thoughts on it.

@chemicL
Copy link
Collaborator

chemicL commented Apr 23, 2024

@ppkarwasz thanks for taking the time to consider this issue. I've seen ThreadContextMap used in ServiceTalk.

From the perspective of the context-propagation library, we're aiming to provide a general solution that would be well aligned with other provider libraries that rely on ThreadLocal state, like SLF4J, Brave, OpenTelemetry, Logback and bridging that with "container" map-like structures, such as Reactor's Context.

ThreadContextMap is unfortunately limited to Log4J2. Do you have thoughts about generalizing the approach in the face of ScopedValue?

@ppkarwasz
Copy link

@chemicL,

ThreadContextMap is unfortunately limited to Log4J2. Do you have thoughts about generalizing the approach in the face of ScopedValue?

To be precise, ThreadContextMap is only used by the Log4j Core logging backend. In a similar way MDC is only used by Logback (through the MDCAdapter interface) and JBoss Log Manager uses his own MDCProvider interface.

For these 3 services you can use the same approach: they can write their data to a common storage, which can be a map of maps backed by a ScopedValue.

This common storage does not necessarily needs to be provided by context-propagation, but context-propagation needs to integrate with it. In the current state of affairs I would choose OpenTelemetry's Baggage API as common storage, although it has the disadvantage of propagating between JVMs. OpenTelemetry however has a proposal for an intra-JVM storage: open-telemetry/oteps#207

From the perspective of Log4j, we must provide a ThreadContext implementation for backward compatibility, so that 15 years old applications, whose concurrency model is based on new Thread, continue to work. However I am not eager to introduce ScopedValue in our implementation anytime soon.

On the other hand I would gladly maintain a ThreadContextMap implementation that stores its data directly in the appropriate carrier for each concurrency library.

@marcingrzejszczak
Copy link
Contributor

I've started a discussion about it in the loom dev mailing list. Feel free to chime in @ppkarwasz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

6 participants