Dmitry, Thanks for your example! The approach looks similar to "problematic" on used in jdk described in [1]. Fortunately, in our case buffer is quite limited. On the other hand we always should be cautious because Ignite code frequently is running on user threads (e.g. it is typical for SQL).
[1] http://www.evanjones.ca/java-bytebuffer-leak.html 2018-09-13 9:28 GMT+03:00 Alexey Goncharuk <alexey.goncha...@gmail.com>: > Maxim, > > If multiple instances of Ignite is started in the same JVM and a user > thread will access first one instance of Ignite, then another, you will end > up with the static thread local holding the last WAL pointer from the > second grid. This is possible, for example, when a user thread commits a > transaction or runs an atomic update on a data node. Any access of the > first Ignite instance will have an invalid thread-local value. > > вт, 11 сент. 2018 г. в 13:29, Maxim Muzafarov <maxmu...@gmail.com>: > > > 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 > > > -- Best regards, Ivan Pavlukhin
