apache / apache/lucene

Improvement for CloseableThreadLocal [LUCENE-10519]

Open
#11,555 3 comments 0 reactions 0 assignees View on GitHub
affects-version:8.10.1 affects-version:8.11.1 affects-version:8.9 legacy-jira-label:CloseableThreadLocal legacy-jira-priority:Critical module:core/other type:bug
Dominant language
Java
Stars
3.6k
Forks
1.4k
Avg merge
2d 11h
Merged PRs (30d)
88

Description

## Problem

----
{**}org.apache.lucene.util.CloseableThreadLocal{**}(which is using {**}ThreadLocal<WeakReference{**}) may still have a flaw under G1GC. There is a single ThreadLocalMap stored for each thread, which all ThreadLocals share, and that master map only periodically purges stale entries. When we close a CloseableThreadLocal, we only take care of the current thread right now, others will be taken care of via the WeakReferences. Under G1GC, the WeakReferences of other threads may not be recycled even after several rounds of mix-GC. The ThreadLocalMap may grow very large, it can take an arbitrarily long amount of CPU and time to iterate the things you had stored in it.

Hot thread of elasticsearch:

```java
::: {xxxxxxxxx}{lCj7LcVnT328KHcJRd57yg}{WPiNCbk0R0SIKxg4-w3wew}{xxxxxxxx}{xxxxxxxx}
Hot threads at 2020-04-12T05:25:10.224Z, interval=500ms, busiestThreads=3, ignoreIdleThreads=true:

105.3% (526.5ms out of 500ms) cpu usage by thread 'elasticsearch[xxxxxxxx][bulk][T#31]'
10/10 snapshots sharing following 34 elements
java.lang.ThreadLocal$ThreadLocalMap.expungeStaleEntry(ThreadLocal.java:627)
java.lang.ThreadLocal$ThreadLocalMap.remove(ThreadLocal.java:509)
java.lang.ThreadLocal$ThreadLocalMap.access$200(ThreadLocal.java:308)
java.lang.ThreadLocal.remove(ThreadLocal.java:224)
java.util.concurrent.locks.ReentrantReadWriteLock$Sync.tryReleaseShared(ReentrantReadWriteLock.java:426)
java.util.concurrent.locks.AbstractQueuedSynchronizer.releaseShared(AbstractQueuedSynchronizer.java:1349)
java.util.concurrent.locks.ReentrantReadWriteLock$ReadLock.unlock(ReentrantReadWriteLock.java:881)
org.elasticsearch.common.util.concurrent.ReleasableLock.close(ReleasableLock.java:49)
org.elasticsearch.index.engine.InternalEngine.$closeResource(InternalEngine.java:356)
org.elasticsearch.index.engine.InternalEngine.delete(InternalEngine.java:1272)
org.elasticsearch.index.shard.IndexShard.delete(IndexShard.java:812)
org.elasticsearch.index.shard.IndexShard.applyDeleteOperation(IndexShard.java:779)
org.elasticsearch.index.shard.IndexShard.applyDeleteOperationOnReplica(IndexShard.java:750)
org.elasticsearch.action.bulk.TransportShardBulkAction.performOpOnReplica(TransportShardBulkAction.java:623)
org.elasticsearch.action.bulk.TransportShardBulkAction.performOnReplica(TransportShardBulkAction.java:577)
```

## Solution

----
This bug does not reproduce under CMS. It can be reproduced under G1GC always.

In fact, **CloseableThreadLocal** doesn't need to store entry twice in the hardRefs And ThreadLocals. Remove ThreadLocal from CloseableThreadLocal so that we would not be affected by the serious flaw of Java's built-in ThreadLocal. 
## See also

----

![image-2022-04-27-16-40-34-796.png](https://apache.github.io/lucene-jira-archive/attachments/LUCENE-10519/image-2022-04-27-16-40-34-796.png)

---
Migrated from [LUCENE-10519](https://issues.apache.org/jira/browse/LUCENE-10519) by Lucifer Boice, updated Apr 27 2022
Environment:
```
Elasticsearch v7.16.0

OpenJDK v11
```

Attachments: [image-2022-04-27-16-40-34-796.png](https://apache.github.io/lucene-jira-archive/attachments/LUCENE-10519/image-2022-04-27-16-40-34-796.png), [image-2022-04-27-16-40-58-056.png](https://apache.github.io/lucene-jira-archive/attachments/LUCENE-10519/image-2022-04-27-16-40-58-056.png), [image-2022-04-27-16-41-55-264.png](https://apache.github.io/lucene-jira-archive/attachments/LUCENE-10519/image-2022-04-27-16-41-55-264.png)
Pull requests: https://github.com/apache/lucene/pull/816

Contributor guide

Open the contributing guide

Research direction

Start by reading the CloseableThreadLocal implementation and the linked pull request, then compare its ThreadLocal and hard-reference handling with the G1GC failure described here. Done means CloseableThreadLocal no longer relies on Java's problematic ThreadLocal storage while preserving its close and reference-management behavior.

Written by the indexing model from the issue text.

Assessment

Tech stack
java
Domain
backend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
25/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.