Flow: Questionable use of instance ThreadLocal in Composite

[fluorumlabs]

Fix #8529 introduced non-static ThreadLocal which is used to prevent stack overflow due to Composite.getContent() reentry. However, If getContent() can be called only when UI is locked, ThreadLocal is not needed. And if getContent() can be called by several threads, then initContent() might be called twice and different threads will receive different instances of the underlying component (because of lack of synchronization)

[denis-anisimov]

ThreadLocal is used for clear semantic: for _this_ instance of a Component for _the current_ stacktrace logic should be applied.
This is the booletproof way of checking that one method has been called inside another by the stacktrace:

private ThreadLocal<Boolean> isInsideMethod1 = new ThreadLocal<>();

private void method1(){
isInsideMethod1.set(true);
try{
....
method2();
....
}
finally{
isInsideMethod1.set(false);
}
}

private void method2(){
if ( isInsideMethod1.get(){
  // do something only if: 
 // the method is called for the same instance
 // the method is called inside the same stacktrace
}
}

so :

• static ThreadLocal has simply totally no any sense.
• no need to care about whether UI is locked or not: this is totally different thing. This is additional assumption which needs to be either checked or forced. And it's too much efforts to check or force this assumption . ThreadLocal will do what it's intended for regardless of any assumption.
• avoiding ThreadLocal makes a field a state of the _object_ but this is rather something which is 1:1 to stacktrace&object, it's more a state of stacktrace applied to the object. So semantically a state of the object is wrong. ThreadLocal makes it semantically accurate.

So in the result : the fix doesn't care about any thread safety: it uses the correct semantic of ThreadLocal.
The semantic says that the call is done in the same thread which means the same stacktrace considering the impl.
Everything UI lock related is totally different question with totally different semantic and out of topic.

If you see an issue please make a test which proves that something is broken.
I don't see any issues and consider the current impl as the only semantically correct way.

[mvysny]

ThreadLocal doesn't care about stack trace, so that argument is moot. Well, maybe loosely; but the main point is to store different values for different threads. However, there is just one thread.

The same code would work exactly the same if ThreadLocal was simply replaced by boolean. Therefore, using boolean is simpler. You can continue using transitive, to tell that it's not part of a persistent object state.

Using ThreadLocal implies that the code handles multiple threads. That is however not the case: UI code only runs in one thread. Using ThreadLocal is therefore not bullet-proofing - it is unnecessary complex and misguides anyone who tries to read the code.

[fluorumlabs]

no need to care about whether UI is locked or not: this is totally different thing

Now that you mentioned it... Since there are no checked or enforced assumptions about concurrency, let's assume, that getContent() CAN be called by several threads. In that case we have missing synchronization for lazy initialization of content.

If we assume, on the other hand, that getContent() can be called only from the single thread, then synchronization is not needed, but neither is ThreadLocal, because there can be no other thread/stack.

In other words, there are two possible resolutions:

1. Add necessary DCL if concurrency is allowed
2. Refactor the code and remove ThreadLocal

Note about semantic correctness: from the perspective of getContent()/initContent(), Composite can has three states:

1. Not initialized (content is null, initContent() is not called)
2. Initializing in progress (content is null, initContent() call is in progress)
3. Initialized (content is initialized)

[mvysny]

@fluorumlabs no need to support multiple threads - all Vaadin UI components are thread-unsafe since the UI can be accessed by one thread only. Therefore I wouldn't make Composite act differently - there is no need since it's a UI code.

Making Composite thread-safe would be imho a bad thing: it would raise confusion as to why this particular code has been made thread-safe while other UI-related code is thread-unsafe.

[fluorumlabs]

@mvysny That's exactly the point. I wanted to emphasize that the chosen solution is incorrect in both cases (concurrent and non-concurrent)

[denis-anisimov]

Don't see any point to waste time on personal opinions.
I totally disagree with both of you.
You disagree with me. I'm OK with this. It's just an opinion.

Let's keep this open forever.

[fluorumlabs]

Even the exception that is thrown when the attempt to call getContent() from inside of initContent() is made is IllegalStateException`

[fluorumlabs]

Opinions aside, could you @denis-anisimov explain, why simple private transient boolean isContentInitializing is not sufficient in this case? Since it's primitive, you won't even need readObject method -- it will be false when an empty instance is created.

[denis-anisimov]

It's sufficient _assuming_ the object is not ever used by several threads.
It works _assuming_ the object is never used by several threads simultaneously.

I've explained the reason why ThreadLocal is logically correct here in details : https://github.com/vaadin/flow/issues/9160#issuecomment-707138402

I can't explain it better.
One important point one more time: the fix doesn't solve _any_ thread safety issue. ThreadLocal doesn't solve such issues anyhow at all. It's you who introduced this question here.
The code _which prevents calling the same method recursively_ will work regardless of any assumption correctly (regardless Vaadin, UI, Component, whatever). Entities should not be multiplied without necessity.

[fluorumlabs]

It's sufficient _assuming_ the object is not ever used by several threads.

If that assumption is not valid, the current fix is obviously missing proper Double Checked Locking. If it's valid, it's overengineered/overgeneralized to support the case that will never happen.