What about only allowing primitive types and standard containers, i.e., map and collection? We can easily validate that as we do while serializing JSON.
On Fri, 5 Mar 2021, 17:55 Ralph Goers <[email protected]> wrote: > That is the same as remove(). > > Think about how Tomcat works. It creates a Thread pool for all incoming > requests. They are no threads dedicated to a single application. > Applications are supposed to clear any ThreadLocals when the execution on a > Thread ends otherwise the values will be there when the next request comes > along. So we tell them to clear the MDC at the end of processing the > Request. Hopefully they do. But we have no way to ensure they don’t. > > We can’t blindly go clearing ThreadLocals when an app is undeployed > because we have no idea what Threads were last used by that application. > Even if we did, we have no way of unsetting a value in any thread but the > current one. > > Also, remember that this is not necessarily shutdown. Tomcat allows apps > to undeploy and redeploy. OSGi does the same thing in even more weird ways. > > The bottom line is you just can’t let user’s put arbitrary objects in the > ThreadContext. > > Ralph > > > On Mar 5, 2021, at 9:45 AM, Volkan Yazıcı <[email protected]> > wrote: > > > > What about simply calling threadLocal.set(null) at shutdown? > > > > On Fri, 5 Mar 2021, 17:30 Ralph Goers <[email protected]> > wrote: > > > >> The remove() method would have to be called on every thread. Setting the > >> ThreadLocal to null will cause massive problems if Log4j is installed in > >> the parent’s ClassLoader as you would be killing the ThreadLocal for all > >> applications, not just the one being underplayed. > >> > >> ClassLoaders are fun. > >> > >> Ralph > >> > >>> On Mar 5, 2021, at 9:22 AM, Matt Sicker <[email protected]> wrote: > >>> > >>> There's a remove() method on ThreadLocal. > >>> > >>> On Fri, 5 Mar 2021 at 10:13, Volkan Yazıcı <[email protected]> > >> wrote: > >>>> > >>>> Maybe thinking naively but... Can't we just set the ThreadLocal to > null > >> at > >>>> shutdown? > >>>> > >>>> On Fri, Mar 5, 2021 at 5:04 PM Ralph Goers < > [email protected]> > >>>> wrote: > >>>> > >>>>> I am having a difficult time locating the conversation with Ceki but > it > >>>>> happened years ago either on the SLF4J or Logback lists. The > >> serialization > >>>>> of objects in the MDC was just one issue. As I recall the larger > issue > >>>>> related to ClassLoaders. > >>>>> > >>>>> We don’t have problems putting objects into a ThreadLocal because we > >> are > >>>>> careful about what we put there and how long it stays. In other > words, > >> we > >>>>> control the ThreadLocal and its contents. With the ThreadContext we > >> control > >>>>> the ThreadLocal but we don’t control its contents. This means that if > >> an > >>>>> application tries to shutdown any objects left in the ThreadLocal > that > >> are > >>>>> owned by the ClassLoader of the application will cause the > >> undeployment of > >>>>> the application to fail. S I recall that is the main reason Ceki > >> decided to > >>>>> only support Strings in the MDC when he created SLF4J. When I created > >> the > >>>>> Log4j 2 API I couldn’t find a flaw in that reasoning. > >>>>> > >>>>> Theoretically it would be possible to support primitive objects and > any > >>>>> objects owned by a parent ClassLoader so long as they don’t reference > >>>>> anything owned by the application ClassLoader, but validating that > >> would be > >>>>> a nightmare. The only way I know of to support primitive objects > would > >> be > >>>>> to provide overloaded methods for each of the types we would want to > >>>>> support. > >>>>> > >>>>> Ralph > >>>>> > >>>>>> On Mar 5, 2021, at 5:57 AM, Remko Popma <[email protected]> > >> wrote: > >>>>>> > >>>>>> I think so yes. > >>>>>> But a quick read doesn’t show drawbacks. > >>>>>> Maybe I’ll remember later. > >>>>>> > >>>>>> > >>>>>>> On Mar 5, 2021, at 21:54, Volkan Yazıcı <[email protected]> > >>>>> wrote: > >>>>>>> > >>>>>>> Are you referring to LOG4J2-1648 > >>>>>>> <https://issues.apache.org/jira/browse/LOG4J2-1648>? > >>>>>>> > >>>>>>>> On Fri, Mar 5, 2021 at 1:45 PM Remko Popma <[email protected] > > > >>>>> wrote: > >>>>>>>> > >>>>>>>> There should be an existing JIRA that contains fairly extensive > >>>>> analysis > >>>>>>>> on this topic. > >>>>>>>> > >>>>>>>> There are some implications/drawbacks, can’t remember off the top > >> of my > >>>>>>>> head. > >>>>>>>> > >>>>>>>> Would need to look at the ticket but no time now, maybe tomorrow. > >>>>>>>> > >>>>>>>>>> On Mar 5, 2021, at 19:45, Volkan Yazıcı < > [email protected]> > >>>>> wrote: > >>>>>>>>> > >>>>>>>>> Hello, > >>>>>>>>> > >>>>>>>>> In the past couple of months I have received two complaints from > >>>>> people > >>>>>>>> who > >>>>>>>>> want to put non-String values into the ThreadContextMap. > >>>>>>>>> ThreadContext.put*() methods only accept String values, whereas 2 > >> of > >>>>> the > >>>>>>>> 3 > >>>>>>>>> backend maps (GarbageFreeSortedArrayThreadContextMap, > >>>>>>>>> CopyOnWriteSortedArrayThreadContextMap) and the exposed > >>>>> ReadOnlyStringMap > >>>>>>>>> interface support non-String values. (The one out of 3, > >>>>>>>>> DefaultThreadContextMap, employed when thread locals are > disabled, > >>>>> only > >>>>>>>>> supports String values.) I want to improve this situation by > >>>>> supporting > >>>>>>>>> Object values in ThreadContextMap. Is this a known issue? What > >> would > >>>>> be > >>>>>>>> the > >>>>>>>>> implications of extending ThreadContextMap? I will appreciate > some > >>>>>>>> guidance > >>>>>>>>> on this issue. > >>>>>>>>> > >>>>>>>>> Kind regards. > >>>>>>>> > >>>>>> > >>>>> > >>>>> > >>>>> > >>> > >> > >> > >> > > >
