Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 1.1.0
    • Labels:
      None
    • Environment:

      Operating System: other
      Platform: Other

      Description

      Hi,

      the LogFactory class is not thread-safe. All access to the factories variable
      should be in synchronized code. This is sometimes the case (like in the
      LogFactory.releasXXX() methods), but in the LogFactory.getFactory() method,
      this variable is accessed in a non-synchronized way.

      regards,
      Maarten

        Activity

        Hide
        Simon Kitching added a comment -

        I don't believe this issue is valid. The Hashtable javadoc specifies that this
        class is internally synchronized (ie already thread-safe) which will prevent
        race conditions between concurrent calls to get/put.

        There is potentially an issue with the removeAll method, as a
        ConcurrentModificationException could occur if one thread is in removeAll while
        another is adding an entry to the table. However removeAll should only ever be
        called by the container during container shutdown so this is not expected to
        ever be possible in practice.

        Adding synchronisation everywhere just to avoid a theoretical conflict with
        removeAll isn't a good idea; that's a significant performance hit.

        If I've got this wrong, please let me know. Otherwise I will close this issue in
        a couple of days.

        Show
        Simon Kitching added a comment - I don't believe this issue is valid. The Hashtable javadoc specifies that this class is internally synchronized (ie already thread-safe) which will prevent race conditions between concurrent calls to get/put. There is potentially an issue with the removeAll method, as a ConcurrentModificationException could occur if one thread is in removeAll while another is adding an entry to the table. However removeAll should only ever be called by the container during container shutdown so this is not expected to ever be possible in practice. Adding synchronisation everywhere just to avoid a theoretical conflict with removeAll isn't a good idea; that's a significant performance hit. If I've got this wrong, please let me know. Otherwise I will close this issue in a couple of days.
        Hide
        Nathan Beyer added a comment -

        The Hashtable is synchronized using synchronized keywords on the methods, which
        means that that lock object is the instance itself. As such, everything is
        waiting on the same lock, so releaseAll is safe because it synchronizes on the
        instance before manipulating anything and the other mutation points, like the
        cacheFactory method would be blocked during a Hashtable.put if releaseAll were
        in process.

        Show
        Nathan Beyer added a comment - The Hashtable is synchronized using synchronized keywords on the methods, which means that that lock object is the instance itself. As such, everything is waiting on the same lock, so releaseAll is safe because it synchronizes on the instance before manipulating anything and the other mutation points, like the cacheFactory method would be blocked during a Hashtable.put if releaseAll were in process.
        Hide
        Maarten Coene added a comment -

        The releaseXXX() methods are safe, but the getFactory() is not.

        It is possible that 2 threads calling the getFactory() method will receive
        different LogFactory instances for the same classloader. I'm not saying that
        you should make getFactory() synchronized, but you should say more explicitly
        in the javadoc that the getFactory() method not necessarely returns the same
        factory instance in a multithreaded environment.

        Consider the following simple scenario:
        No Factory has been created yet. 2 Different threads have just been started.
        Thread1 calls LogFactory.getFactory(). This method will look in the cache, but
        since it is the first time the factory is retrieved, the cache will return
        null and the thread will continue creating the factory instance. In the middle
        of the getFactory() method, thread1 is suspended and thread2 becomes active.
        Thread2 calls getFactory(). The cache still returns null! (Thread1 didn't put
        the factory instance in the cache yet). So thread2 will continue creating a
        new factory instance. Both threads will end up having their own instance of a
        LogFactory. One of them is cached, the other not.

        In most cases, this is no big deal: it's just an instance of a factory that
        has been created too much. This won't cause problems in > 99% of the
        applications.

        But it could get worse: if you now call LogFactory.releaseAll(), one instance
        of the factory won't be released at all (because it's not present in the
        cache)! If your factory implementation consumes a lot of resources, this could
        cause a serious negative impact on your system.

        regards,
        Maarten

        Show
        Maarten Coene added a comment - The releaseXXX() methods are safe, but the getFactory() is not. It is possible that 2 threads calling the getFactory() method will receive different LogFactory instances for the same classloader. I'm not saying that you should make getFactory() synchronized, but you should say more explicitly in the javadoc that the getFactory() method not necessarely returns the same factory instance in a multithreaded environment. Consider the following simple scenario: No Factory has been created yet. 2 Different threads have just been started. Thread1 calls LogFactory.getFactory(). This method will look in the cache, but since it is the first time the factory is retrieved, the cache will return null and the thread will continue creating the factory instance. In the middle of the getFactory() method, thread1 is suspended and thread2 becomes active. Thread2 calls getFactory(). The cache still returns null! (Thread1 didn't put the factory instance in the cache yet). So thread2 will continue creating a new factory instance. Both threads will end up having their own instance of a LogFactory. One of them is cached, the other not. In most cases, this is no big deal: it's just an instance of a factory that has been created too much. This won't cause problems in > 99% of the applications. But it could get worse: if you now call LogFactory.releaseAll(), one instance of the factory won't be released at all (because it's not present in the cache)! If your factory implementation consumes a lot of resources, this could cause a serious negative impact on your system. regards, Maarten
        Hide
        rdonkin@apache.org added a comment -

        I agree that the performance advantages outweight the downside but it should be
        documented. I've committed a note to the getFactory method javadocs which should
        be suitable.

        Please check and if not satisfactory the please reopen and attach a patch for
        the documentation.

        Robert

        Show
        rdonkin@apache.org added a comment - I agree that the performance advantages outweight the downside but it should be documented. I've committed a note to the getFactory method javadocs which should be suitable. Please check and if not satisfactory the please reopen and attach a patch for the documentation. Robert

          People

          • Assignee:
            Unassigned
            Reporter:
            Maarten Coene
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development