ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dmitriy Pavlov <dpavlov....@gmail.com>
Subject Re: Class field ThreadLocal. Why not static?
Date Wed, 12 Sep 2018 17:22:58 GMT
Hi Ivan,

For example, I remember thread local buffer in our old WAL implementation:
org.apache.ignite.internal.processors.cache.persistence.wal.FsyncModeFileWriteAheadLogManager#tlb

Sincerely,
Dmitriy Pavlov

вт, 11 сент. 2018 г. в 13:05, Павлухин Иван <vololo100@gmail.com>:

> 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
>

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