Commons BeanUtils
  1. Commons BeanUtils
  2. BEANUTILS-146

[bean] DateLocaleConverter is not thread-safe

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Critical Critical
    • Resolution: Fixed
    • Affects Version/s: 1.6
    • Fix Version/s: None
    • Component/s: None
    • Labels:
      None
    • Environment:

      Operating System: All
      Platform: All

      Description

      It uses a SimpleDateFormat allocated during class-<init>, so multiple thread
      cannot access this class.

        Activity

        Hide
        rdonkin@apache.org added a comment -

        hmmm....

        i take it that you are referring to:

        <code>
        public class DateLocaleConverter extends BaseLocaleConverter {

        // ----------------------------------------------------- Instance Variables

        /** All logging goes through this logger */
        private static Log log = LogFactory.getLog(DateLocaleConverter.class);

        /** The Date formatter */
        private SimpleDateFormat formatter = getPattern(pattern, locale);
        </code>

        right?

        i'm not convinced by your assertion that this is not thread safe. i think that constructors are by their
        nature non-reentrant (for an instance) and effectively act as if the method (and any called either directly
        or indirectly from within a constructor) is synchronized.

        of course, i'd be happy to be proved wrong if you can supply a solid reference or a good test case
        demonstrating the problem.

        Show
        rdonkin@apache.org added a comment - hmmm.... i take it that you are referring to: <code> public class DateLocaleConverter extends BaseLocaleConverter { // ----------------------------------------------------- Instance Variables /** All logging goes through this logger */ private static Log log = LogFactory.getLog(DateLocaleConverter.class); /** The Date formatter */ private SimpleDateFormat formatter = getPattern(pattern, locale); </code> right? i'm not convinced by your assertion that this is not thread safe. i think that constructors are by their nature non-reentrant (for an instance) and effectively act as if the method (and any called either directly or indirectly from within a constructor) is synchronized. of course, i'd be happy to be proved wrong if you can supply a solid reference or a good test case demonstrating the problem.
        Hide
        Luca Masini added a comment -

        From Sun's J2SDK JavaDoc, v 1.4.1_01:

        Class: SimpleDateFormat

        "Synchronization

        Date formats are not synchronized. It is recommended to create separate format
        instances for each thread. If multiple threads access a format concurrently, it
        must be synchronized externally. "

        Is this a strong enough reference ?

        Show
        Luca Masini added a comment - From Sun's J2SDK JavaDoc, v 1.4.1_01: Class: SimpleDateFormat "Synchronization Date formats are not synchronized. It is recommended to create separate format instances for each thread. If multiple threads access a format concurrently, it must be synchronized externally. " Is this a strong enough reference ?
        Hide
        rdonkin@apache.org added a comment -

        ok.

        so it's the use of SimpleDateFormat that's problematic in a multithreaded environment that's the issue.

        what's interesting is that the syncronization requirement only appears after java 1.2. i'm not sure
        whether the 1.4.1 class is now not guaranteed to be threadsafe or whether it's always been that way
        (but never been documented, i'm not sure).

        i'll add some synchronization some time soonish.

        Show
        rdonkin@apache.org added a comment - ok. so it's the use of SimpleDateFormat that's problematic in a multithreaded environment that's the issue. what's interesting is that the syncronization requirement only appears after java 1.2. i'm not sure whether the 1.4.1 class is now not guaranteed to be threadsafe or whether it's always been that way (but never been documented, i'm not sure). i'll add some synchronization some time soonish.
        Hide
        Luca Masini added a comment -

        It has been always been NOT multi-thread safe, the fact is that they documented
        it starting from 1.4, after a bug opened at java.sun.com.
        Please, don't use synchronization, but instantiate a new SimpleDataFormat in the
        method that use it. I have a heavy use financial information server that
        suffered from that bug in jdk1.1 and testing, we discovered that the better way
        is not to synchronize, but to instantiate new.
        Ciao.

        Show
        Luca Masini added a comment - It has been always been NOT multi-thread safe, the fact is that they documented it starting from 1.4, after a bug opened at java.sun.com. Please, don't use synchronization, but instantiate a new SimpleDataFormat in the method that use it. I have a heavy use financial information server that suffered from that bug in jdk1.1 and testing, we discovered that the better way is not to synchronize, but to instantiate new. Ciao.
        Hide
        rdonkin@apache.org added a comment -

        bit odd that - but i'll take your work for it and create a new one each time. i'll probably add some
        documentation as well to explain why it's like that.

        Show
        rdonkin@apache.org added a comment - bit odd that - but i'll take your work for it and create a new one each time. i'll probably add some documentation as well to explain why it's like that.
        Hide
        rdonkin@apache.org added a comment -

        Committed the fix you suggested. Thanks for your help in spotting and resolving this one. I'd
        appreciated it if you could find time to give the new code a look over and ensure that it's ok.

        Whilst fixing this bug I've become aware that the quality of the testing of code in the localized beanutils
        leaves a lot to be desired and there are a lot of missing features. This is an area that really needs
        working on before the next release can happen. Alas, this is not a particular priority for me at the
        moment but I'd be willing to make time to review and commit good contributions in this area.

        Show
        rdonkin@apache.org added a comment - Committed the fix you suggested. Thanks for your help in spotting and resolving this one. I'd appreciated it if you could find time to give the new code a look over and ensure that it's ok. Whilst fixing this bug I've become aware that the quality of the testing of code in the localized beanutils leaves a lot to be desired and there are a lot of missing features. This is an area that really needs working on before the next release can happen. Alas, this is not a particular priority for me at the moment but I'd be willing to make time to review and commit good contributions in this area.

          People

          • Assignee:
            Unassigned
            Reporter:
            Luca Masini
          • Votes:
            0 Vote for this issue
            Watchers:
            0 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development