RFR: 8258776: ThreadLocal#initialValue() Javadoc is unaware of ThreadLocal#withInitial()

[Alan Bateman]

On Wed, 4 Jan 2023 14:37:13 GMT, Jaikiran Pai <jpai at openjdk.org> wrote:

> Can I please get a review of this doc only change which addresses the javadoc issue noted in https://bugs.openjdk.org/browse/JDK-8258776? 
> 
> As noted in that issue, the `ThreadLocal.initialValue()` API javadoc suggests subclassing `ThreadLocal` and overriding the `initialValue()` method instead of recommending the `withInitial` method that was added in Java 8.
> 
> The commit here updates the javadoc of `initialValue()` method to note that `withInitial` method is available for use if the programmer wants to provide an initial value. The updated javadoc continues to note that the `ThreadLocal` can be subclassed and the method overriden as the other way of providing an initial value. This change now uses the `@implSpec` construct to state this default behaviour of the `initialValue()` method.
> 
> Additionally, the `get()` method's javadoc has also been updated to account for the semantics of `initialValue()`. In fact, in its current form, before the changes in this PR, the `get()` method states:
> 
>> If the current thread does not support thread locals then
>> this method returns its {@link #initialValue} (or {@code null}
>> if the {@code initialValue} method is not overridden).
> 
> which isn't accurate, since the `get()` will return a non-null value if the ThreadLocal was constructed through `ThreadLocal.withInitial`. Here's a trivial code which contradicts this current javadoc:
> 
> 
> public class Test {
>   public static void main(final String[] args) throws Exception {
>     Thread.ofPlatform().name("hello").allowSetThreadLocals(false).start(() -> {
> 	var t = ThreadLocal.withInitial(() -> 2);
> 	System.out.println("Thread local value is " + t.get());
>     });
>   }
> }
> 
> Running this with `java --enable-preview --source 21 Test.java` returns:
> 
> Thread local value is 2
> 
> 
> The updated javadoc of `get()` in this PR now matches the behaviour of this method. I believe this will need a CSR, which I'll open once we have an agreement on these changes.
> 
> Furthermore, the class level javadoc of `ThreadLocal` has an example of providing an initial value which uses the subclassing strategy. I haven't updated that part. Should we update that to show the `withInitialValue` usage instead (and maybe convert it to a snippet)?

src/java.base/share/classes/java/lang/ThreadLocal.java line 128:

> 126:      * value other than {@code null}, then either {@code ThreadLocal} can be
> 127:      * subclassed and this method overridden or {@link ThreadLocal#withInitial(Supplier)} method
> 128:      * can be used to construct a {@code ThreadLocal}.

This looks okay but maybe slightly better to say "or the method withInitial(Supplier) can be used ...". 

I think you can probably reflow this paragraph to avoid having a mix of short and long lines.

-------------

PR: https://git.openjdk.org/jdk/pull/11846