ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Alexey Goncharuk <alexey.goncha...@gmail.com>
Subject Re: Class field ThreadLocal. Why not static?
Date Thu, 13 Sep 2018 06:28:22 GMT
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 <maxmuzaf@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 Павлухин Иван <vololo100@gmail.com> wrote:
>
> > Dmitriy,
> >
> > Could you point to some piece of code implementing described pattern?
> >
> > 2018-09-11 13:02 GMT+03:00 Павлухин Иван <vololo100@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.goncharuk@gmail.com
> > >:
> > >
> > >> Ivan,
> > >>
> > >> Can you elaborate on the issue with the thread local cleanup you've
> > faced?
> > >>
> > >> вт, 11 сент. 2018 г. в 9:13, Павлухин Иван <vololo100@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.goncharuk@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 <maxmuzaf@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
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message