Alexey, Ivan, Agree. Keeping strong references to the Thread object is the source of memory leak with ThreadLocals variables and the values that it stores. ThreadLocalMap is bound to the Thread lifespan [1], so I think when we are using everything right all will be GC'ed correctly. Is this memory leaks with ThreadLocal's you mean, Alexey? If not, please, share your example.
Also, agree that these usages should be bound to the component lifespan. But for `FileWriteAheadLogManager` I think this variable used not semantically right. I've dumped all threads (total ~49 threads) that are using `lastWalPtr` in `FileWriteAheadLogManager`. For instance, * exchange-worker-#40%wal.IgniteWalRecoveryTest0% * sys-#148%wal.IgniteWalRecoveryTest1% * db-checkpoint-thread-#129%wal.IgniteWalRecoveryTest2% Suppose everything would be OK here for `static` and `non-static` case of ThreadLocal. [1] http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/tip/src/share/classes/java/lang/Thread.java#l760 On Tue, 11 Sep 2018 at 13:05 Павлухин Иван <vololo...@gmail.com> wrote: > Dmitriy, > > Could you point to some piece of code implementing described pattern? > > 2018-09-11 13:02 GMT+03:00 Павлухин Иван <vololo...@gmail.com>: > > > Alex, > > > > ThreadLocal subclass is used in IgniteH2Indexing for simple access to H2 > > Connection from current thread. Such subclass has a capability to create > > connection if one does not exist, so obtaining connection is merely > > ThreadLocal.get. Also there are scheduled routines to cleanup connections > > and associated with them statement cache after some expiration time. For > > that reason Map<Thread, H2ConnectionWrapper> is maintained. As query can > > run on user thread we need to cleanup mentioned map to avoid a leak when > > Thread is terminated. So we need to check thread status in cleanup > routines > > and remove entries for terminated Threads. And historically there was no > > cleanup for terminated threads and leak was possible. And also great care > > must be taken in order to avoid cyclic reference between ThreadLocal > > instance and a stored value. Which easily could occur if the stored value > > is covered by multiple layers of abstraction. > > > > And I am describing some historical state. Now machinery in > IgniteH2Indexing > > is even more complex (I hope we will have a chance to improve it). > > > > 2018-09-11 11:00 GMT+03:00 Alexey Goncharuk <alexey.goncha...@gmail.com > >: > > > >> Ivan, > >> > >> Can you elaborate on the issue with the thread local cleanup you've > faced? > >> > >> вт, 11 сент. 2018 г. в 9:13, Павлухин Иван <vololo...@gmail.com>: > >> > >> > Guys, > >> > > >> > As we know ThreadLocal is an instrument which should be used with > great > >> > care. And I recently faced with problems related to proper cleanup of > >> > ThreadLocal which is not needed anymore. In my opinion the best thing > >> (in > >> > ideal world) is to get rid of ThreadLocal where possible, but I guess > >> that > >> > it is quite hard (in real world). > >> > > >> > Also, a question comes to my mind. As ThreadLocal is so common in our > >> code, > >> > could you suggest some guidance or code fragments which address proper > >> > ThreadLocal > >> > lifecycle control and especially cleanup? > >> > > >> > 2018-09-10 12:46 GMT+03:00 Alexey Goncharuk < > alexey.goncha...@gmail.com > >> >: > >> > > >> > > Maxim, > >> > > > >> > > Ignite supports starting multiple instances of Ignite in the same > VM, > >> so > >> > > having static thread locals for the fields you mentioned does not > >> work. > >> > > > >> > > Generally, I think thread-local should be bound to the lifespan of > the > >> > > component it describes. Static thread-locals are hard to clean-up > and > >> > they > >> > > often lead to leaks, so I would rather changed existing static > >> > > thread-locals to be non-static. > >> > > > >> > > --AG > >> > > > >> > > пн, 10 сент. 2018 г. в 11:54, Maxim Muzafarov <maxmu...@gmail.com>: > >> > > > >> > > > Igniters, > >> > > > > >> > > > According to javadoc [1] class ThreadLocal: > >> > > > `ThreadLocal instances are typically private *static* fields in > >> classes > >> > > > that wish to associate state with a thread (e.g., a user ID or > >> > > Transaction > >> > > > ID).` > >> > > > > >> > > > So, AFAIK non-static ThreadLocal usage means as `per thread - per > >> class > >> > > > instance`. What the real cases of using non-static ThreadLocal > class > >> > > fields > >> > > > in Ignite code project? When we need it? > >> > > > > >> > > > In Ignite code project I've found ThreadLocal usage as: > >> > > > - non-static - 67 > >> > > > - static - 68 > >> > > > > >> > > > Back to my example, I've checked FileWriteAheadLogManager. It has: > >> > > > 1) private final ThreadLocal<Boolean> interrupted [2] > >> > > > 2) private final ThreadLocal<WALPointer> lastWALPtr [3] > >> > > > I think both of these fields should be set and used as `static`. > Can > >> > > anyone > >> > > > confirm it? > >> > > > > >> > > > > >> > > > [1] > >> > https://docs.oracle.com/javase/8/docs/api/java/lang/ThreadLocal.html > >> > > > [2] > >> > > > > >> > > > https://github.com/apache/ignite/blob/master/modules/ > >> > > core/src/main/java/org/apache/ignite/internal/processors/ > >> > > cache/persistence/wal/FileWriteAheadLogManager.java#L253 > >> > > > [3] > >> > > > > >> > > > https://github.com/apache/ignite/blob/master/modules/ > >> > > core/src/main/java/org/apache/ignite/internal/processors/ > >> > > cache/persistence/wal/FileWriteAheadLogManager.java#L340 > >> > > > -- > >> > > > -- > >> > > > Maxim Muzafarov > >> > > > > >> > > > >> > > >> > > >> > > >> > -- > >> > Best regards, > >> > Ivan Pavlukhin > >> > > >> > > > > > > > > -- > > Best regards, > > Ivan Pavlukhin > > > > > > -- > Best regards, > Ivan Pavlukhin > -- -- Maxim Muzafarov
