On 1 June 2014 20:19, Romain Manni-Bucau <rmannibu...@gmail.com> wrote: > well it is for sure thread safe. Not sure I get why final and synch would be > mandatory in this particular case (field will maybe be cached by thread but > that's not an issue since the value will be unique).
non-final fields are not guaranteed to be published across threads in the absence of sync. The LogHelper class has two such fields and is therefore not thread-safe. > I have nothing against a revert/reapply. I'll open a thread on logging btw. Thanks. > > Romain Manni-Bucau > Twitter: @rmannibucau > Blog: http://rmannibucau.wordpress.com/ > LinkedIn: http://fr.linkedin.com/in/rmannibucau > Github: https://github.com/rmannibucau > > > 2014-06-01 20:55 GMT+02:00 sebb <seb...@gmail.com>: > >> On 1 June 2014 18:54, Romain Manni-Bucau <rmannibu...@gmail.com> wrote: >> > >> > 2014-06-01 19:45 GMT+02:00 sebb <seb...@gmail.com>: >> >> >> >> PING >> >> >> > >> > Pong, sorry, missed this one. >> > >> >> >> >> On 29 May 2014 03:00, sebb <seb...@gmail.com> wrote: >> >> > On 28 May 2014 18:06, <rmannibu...@apache.org> wrote: >> >> >> Author: rmannibucau >> >> >> Date: Wed May 28 17:06:12 2014 >> >> >> New Revision: 1598071 >> >> >> >> >> >> URL: http://svn.apache.org/r1598071 >> >> >> Log: >> >> >> using reentrant locks instead of old synchronized >> >> > >> >> > -1 >> >> > >> >> > This commit mixes two completely different changes: >> >> > - re-entrant locks >> >> > - addition of LogHelper >> >> > >> >> > I think the LogHelper class is a bad idea. It is also not >> >> > thread-safe. >> >> > If the cache is enabled, then it is not possible to change the >> >> > logging >> >> > level during a run. >> >> > >> > >> > >> > How can it be not thread safe? (it is a constant) >> >> The class is not thread-safe as it has a non-final non-synch variable >> >> > You can disable caching. The main point is jcs tests a lot log debug >> > level >> > and depending your backing impl it can be slow (JUL, Log4j 1.2 for what >> > I >> > tested). There is a property if you want to disable this feature but >> > actually in prod you rarely do it. >> > >> >> I have two objections to the commit. >> >> 1) the commit mixes two completely different changes >> >> 2) the LogHelper class is a bad idea, as it prevents changing the logging >> level. >> >> I suggest reverting the commit and re-applying the locking change on its >> own. >> The logging change really needs to be discussed first. > > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org For additional commands, e-mail: dev-h...@commons.apache.org
