Wicket
  1. Wicket
  2. WICKET-4839

Date converters should use a new instance of DateFormat to be thread safe

    Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: 1.4.19, 6.2.0
    • Fix Version/s: 1.5.9
    • Component/s: wicket
    • Labels:
      None

      Description

      Please consider the linked issue WICKET-4833.

      I had to open a new issue because I cannot attach the quickstart project I've prepared to a closed issue.

        Issue Links

          Activity

          Hide
          SoulSpirit added a comment - - edited

          The attached project demonstrates that instances of DateFormat managed by org.apache.wicket.util.convert.converter.DateConverter are shared between different requests and different sessions, thus DateConverter should be thread-safe.

          The application writes to the console the instance number of the DateFormat instance used by DateConverter, and it's clear it doesn't change refreshing the page or running a different HTTP session, provided the Locale assigned by Wicket to the user doesn't change.

          Show
          SoulSpirit added a comment - - edited The attached project demonstrates that instances of DateFormat managed by org.apache.wicket.util.convert.converter.DateConverter are shared between different requests and different sessions, thus DateConverter should be thread-safe. The application writes to the console the instance number of the DateFormat instance used by DateConverter, and it's clear it doesn't change refreshing the page or running a different HTTP session, provided the Locale assigned by Wicket to the user doesn't change.
          Hide
          Martin Grigorov added a comment -

          From now on ConverterLocator will create a new instance of all date based converters.
          Thanks!

          Show
          Martin Grigorov added a comment - From now on ConverterLocator will create a new instance of all date based converters. Thanks!
          Hide
          SoulSpirit added a comment -

          Sorry if I'm insisting, but your solution isn't optimal: https://git-wip-us.apache.org/repos/asf?p=wicket.git;a=commitdiff;h=7d139086fe0cd036d90fd7a32209776c1653ec5c
          Your fix prevents the user to register its own converters for date related datatypes.

          ConverterLocator is a registry, not a factory. The right way to register a custom converter is to call ConverterLocator#set() , which registers an INSTANCE of the submitted converter in a map. With your fix custom converters aren't looked up from this map, making the class buggy.

          There are to solutions coming to my mind:

          • synchronizing the code that makes use of DateFormat in DateConverter and other similar converters (non optimal)
          • forcing the creation of a new DateFormat object each time DateConverter has to use it (a better one)

          Thanks for your time.

          Show
          SoulSpirit added a comment - Sorry if I'm insisting, but your solution isn't optimal: https://git-wip-us.apache.org/repos/asf?p=wicket.git;a=commitdiff;h=7d139086fe0cd036d90fd7a32209776c1653ec5c Your fix prevents the user to register its own converters for date related datatypes. ConverterLocator is a registry, not a factory. The right way to register a custom converter is to call ConverterLocator#set() , which registers an INSTANCE of the submitted converter in a map. With your fix custom converters aren't looked up from this map, making the class buggy. There are to solutions coming to my mind: synchronizing the code that makes use of DateFormat in DateConverter and other similar converters (non optimal) forcing the creation of a new DateFormat object each time DateConverter has to use it (a better one) Thanks for your time.
          Hide
          Martin Grigorov added a comment -

          Good catch! Thanks for reviewing!

          I improved it by checking first in the registered converters and if there is no match then check for the date types and use the new logic.

          It would be even better if instead of registering the converter itself it was possible to register a provider for the converter but we cannot make API breaks until Wicket 7 so this improvement will have to wait.

          Show
          Martin Grigorov added a comment - Good catch! Thanks for reviewing! I improved it by checking first in the registered converters and if there is no match then check for the date types and use the new logic. It would be even better if instead of registering the converter itself it was possible to register a provider for the converter but we cannot make API breaks until Wicket 7 so this improvement will have to wait.
          Hide
          SoulSpirit added a comment -

          Ok, this way the patch works, but it tricks the user who wants to subclass DateConverter.
          It would appear legitimate to subclass DateConverter overriding #getDateFormat() to make it use a different date format than the hard-coded DateFormat.SHORT, but doing so would make the class unsafe, because the special code in ConverterLocator would not trigger.
          To make it work, the subclass should handle concurrency by itself, and it would sound strange, given that DateConverter doesn't do anything special about it.

          I think it would better making clear that IConverter(s) instances are shared between threads, and making each IConverter implementation handle the concurrency by itself.

          In our specific case of DateConverter, the call to DateFormat#getDateInstance() should be simply replaced by a call to new DateFormat() . Simple and safe.

          A more involved but elegant solution could make use of a ThreadLocal<DateFormat> to "cache" the DateFormat instance, but a simple new DateFormat() would suffice it.

          Thanks again.

          Show
          SoulSpirit added a comment - Ok, this way the patch works, but it tricks the user who wants to subclass DateConverter. It would appear legitimate to subclass DateConverter overriding #getDateFormat() to make it use a different date format than the hard-coded DateFormat.SHORT, but doing so would make the class unsafe, because the special code in ConverterLocator would not trigger. To make it work, the subclass should handle concurrency by itself, and it would sound strange, given that DateConverter doesn't do anything special about it. I think it would better making clear that IConverter(s) instances are shared between threads, and making each IConverter implementation handle the concurrency by itself. In our specific case of DateConverter, the call to DateFormat#getDateInstance() should be simply replaced by a call to new DateFormat() . Simple and safe. A more involved but elegant solution could make use of a ThreadLocal<DateFormat> to "cache" the DateFormat instance, but a simple new DateFormat() would suffice it. Thanks again.
          Hide
          Martin Grigorov added a comment -

          Hi,

          I'm not sure that I understand. A patch would make it more clear, I believe.

          "should be simply replaced by a call to new DateFormat() . Simple and safe."
          Could be, but DateFormat's constructor is not visible (at least in JDK 6), so I'm not sure how you instantiated it. Maybe you use SimpleDateFormat instead...

          Additionally DateConverter creates a new DateFormat instance for each call to #convertToString() so I'm starting wonder whether my change is needed at all. It seems to be OK even without it. Many threads share the same DateConverter instance but since it is stateless (doesn't keep state) and creates new instances of DateFormat for each usage of #convertToXYZ() it should be safe.

          Show
          Martin Grigorov added a comment - Hi, I'm not sure that I understand. A patch would make it more clear, I believe. "should be simply replaced by a call to new DateFormat() . Simple and safe." Could be, but DateFormat's constructor is not visible (at least in JDK 6), so I'm not sure how you instantiated it. Maybe you use SimpleDateFormat instead... Additionally DateConverter creates a new DateFormat instance for each call to #convertToString() so I'm starting wonder whether my change is needed at all. It seems to be OK even without it. Many threads share the same DateConverter instance but since it is stateless (doesn't keep state) and creates new instances of DateFormat for each usage of #convertToXYZ() it should be safe.
          Hide
          SoulSpirit added a comment -

          This patch addresses DateConverter thread-safeness problem.

          Show
          SoulSpirit added a comment - This patch addresses DateConverter thread-safeness problem.
          Hide
          SoulSpirit added a comment -

          The problem isn't DateConverter itself. It is true, it's stateless, but java.text.DateFormat isn't. Its apidoc says:

          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.

          And you can verify it by running the quickstart project I've previously attached.

          Show
          SoulSpirit added a comment - The problem isn't DateConverter itself. It is true, it's stateless, but java.text.DateFormat isn't. Its apidoc says: 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. And you can verify it by running the quickstart project I've previously attached.
          Hide
          Martin Grigorov added a comment -

          I'm not sure why the current code deals with DateFormat.getDateInstance(DateFormat.SHORT, locale) instead of returning a new instance of SimpleDateFormat every time.
          I also think that this method returns instances from a pool. I'm not sure why Emond said that the providers are pooled but the DatFormat instances are not.
          The code in java.text.DateFormat#get(int timeStyle, int dateStyle, int flags, Locale) looks like:

          LocaleServiceProviderPool pool =
          LocaleServiceProviderPool.getPool(DateFormatProvider.class);
          if (pool.hasProviders()) {
          DateFormat providersInstance = pool.getLocalizedObject(
          DateFormatGetter.INSTANCE,
          loc,
          timeStyle,
          dateStyle,
          flags);
          if (providersInstance != null)

          { return providersInstance; }

          }

          So, I agree that org.apache.wicket.util.convert.converter.DateConverter#getDateFormat(Locale) should look like:

          public DateFormat getDateFormat(Locale locale)
          {
          if (locale == null)

          { locale = Locale.getDefault(); }

          return new SimpleDateFormat(getPattern(), locale);
          }

          protected String getPattern()

          { ....}

          And with this improvement the earlier commits in ConverterLocator should be reverted.
          Agree ?

          Show
          Martin Grigorov added a comment - I'm not sure why the current code deals with DateFormat.getDateInstance(DateFormat.SHORT, locale) instead of returning a new instance of SimpleDateFormat every time. I also think that this method returns instances from a pool. I'm not sure why Emond said that the providers are pooled but the DatFormat instances are not. The code in java.text.DateFormat#get(int timeStyle, int dateStyle, int flags, Locale) looks like: LocaleServiceProviderPool pool = LocaleServiceProviderPool.getPool(DateFormatProvider.class); if (pool.hasProviders()) { DateFormat providersInstance = pool.getLocalizedObject( DateFormatGetter.INSTANCE, loc, timeStyle, dateStyle, flags); if (providersInstance != null) { return providersInstance; } } So, I agree that org.apache.wicket.util.convert.converter.DateConverter#getDateFormat(Locale) should look like: public DateFormat getDateFormat(Locale locale) { if (locale == null) { locale = Locale.getDefault(); } return new SimpleDateFormat(getPattern(), locale); } protected String getPattern() { ....} And with this improvement the earlier commits in ConverterLocator should be reverted. Agree ?
          Hide
          SoulSpirit added a comment -

          Exactly.
          In the patch I had to take the pattern from the DateFormat instance because there is no way to emulate the style defined by DateFormat.SHORT with a pattern for SimpleDateFormat (DateFormat.SHORT's pattern is somehow generated very differently for each Locale).
          So, I made it a two-step conversion to keep the maximum date pattern compatibility with the original version of DateConverter. I like your proposal of a #getPattern() method.

          Show
          SoulSpirit added a comment - Exactly. In the patch I had to take the pattern from the DateFormat instance because there is no way to emulate the style defined by DateFormat.SHORT with a pattern for SimpleDateFormat (DateFormat.SHORT's pattern is somehow generated very differently for each Locale). So, I made it a two-step conversion to keep the maximum date pattern compatibility with the original version of DateConverter. I like your proposal of a #getPattern() method.
          Hide
          Martin Grigorov added a comment -

          Now, I see what is the reason to use DateFormat.getDateInstance(DateFormat.SHORT, locale) - http://stackoverflow.com/questions/4594519/how-do-i-get-localized-date-pattern-string.
          SimpleDateFormat is not able to produce a short pattern for all possible locales. I.e. DateFormat.getDateInstance() will not always return SimpleDateFormat and as your patch suggests it will throw a ClassCastException. And with your approach it will return an invalid (for the passed locale) pattern in this case.

          The only safe way is to return a clone of the returned DateFormat instance:

          public DateFormat getDateFormat(Locale locale)
          {
          if (locale == null)

          { locale = Locale.getDefault(); }

          // return a clone because DateFormats are not thread-safe and
          // DateFormat.getDateInstance may return an instance from a pool
          return (DateFormat) DateFormat.getDateInstance(DateFormat.SHORT, locale).clone();
          }

          Another way is

          public DateFormat getDateFormat(Locale locale)
          {
          if (locale == null)
          { locale = Locale.getDefault(); }

          DateFormat dateFormat = DateFormat.getDateInstance(DateFormat.SHORT, locale);
          if (dateFormat instanceof SimpleDateFormat)

          { String pattern = ((SimpleDateFormat) dateFormat).toLocalizedPattern(); dateFormat = new SimpleDateFormat(pattern, locale); }

          return dateFormat;
          }

          i.e. a more explicit way of making a clone for SimpleDateFormat.
          I prefer the first approach.

          Show
          Martin Grigorov added a comment - Now, I see what is the reason to use DateFormat.getDateInstance(DateFormat.SHORT, locale) - http://stackoverflow.com/questions/4594519/how-do-i-get-localized-date-pattern-string . SimpleDateFormat is not able to produce a short pattern for all possible locales. I.e. DateFormat.getDateInstance() will not always return SimpleDateFormat and as your patch suggests it will throw a ClassCastException. And with your approach it will return an invalid (for the passed locale) pattern in this case. The only safe way is to return a clone of the returned DateFormat instance: public DateFormat getDateFormat(Locale locale) { if (locale == null) { locale = Locale.getDefault(); } // return a clone because DateFormats are not thread-safe and // DateFormat.getDateInstance may return an instance from a pool return (DateFormat) DateFormat.getDateInstance(DateFormat.SHORT, locale).clone(); } Another way is public DateFormat getDateFormat(Locale locale) { if (locale == null) { locale = Locale.getDefault(); } DateFormat dateFormat = DateFormat.getDateInstance(DateFormat.SHORT, locale); if (dateFormat instanceof SimpleDateFormat) { String pattern = ((SimpleDateFormat) dateFormat).toLocalizedPattern(); dateFormat = new SimpleDateFormat(pattern, locale); } return dateFormat; } i.e. a more explicit way of making a clone for SimpleDateFormat. I prefer the first approach.
          Hide
          Sven Meier added a comment -

          +1 for just using clone(), it seems the simplest solution. People who are concerned about performance (if this is an issue at all) can always register their own converter.

          Show
          Sven Meier added a comment - +1 for just using clone(), it seems the simplest solution. People who are concerned about performance (if this is an issue at all) can always register their own converter.
          Hide
          SoulSpirit added a comment -

          Speaking about performance, it should be much faster just synchronizing the block, that has the plus to make clear to everyone that DateConverter has to be thread-safe:

          @Override
          public String convertToString(final Date value, final Locale locale)
          {
          final DateFormat dateFormat = getDateFormat(locale);
          if (dateFormat != null)
          {
          synchronized (dateFormat)

          { return dateFormat.format(value); }

          }
          return value.toString();
          }

          Show
          SoulSpirit added a comment - Speaking about performance, it should be much faster just synchronizing the block, that has the plus to make clear to everyone that DateConverter has to be thread-safe: @Override public String convertToString(final Date value, final Locale locale) { final DateFormat dateFormat = getDateFormat(locale); if (dateFormat != null) { synchronized (dateFormat) { return dateFormat.format(value); } } return value.toString(); }
          Hide
          Martin Grigorov added a comment -

          See WICKET-4858. There I tried to consolidate the common code for all Date-based converters in one AbstractDateConverter but clirr-maven-plugin sees problems in applying the change for Wicket 6.x, so it will have to wait for Wicket 7.

          For Wicket 1.5.9/6.3.0 I'm going to revert the change in ConverterLocator and use #clone(). Later this can be optimized if it shows performance problems.

          Show
          Martin Grigorov added a comment - See WICKET-4858 . There I tried to consolidate the common code for all Date-based converters in one AbstractDateConverter but clirr-maven-plugin sees problems in applying the change for Wicket 6.x, so it will have to wait for Wicket 7. For Wicket 1.5.9/6.3.0 I'm going to revert the change in ConverterLocator and use #clone(). Later this can be optimized if it shows performance problems.
          Hide
          Martin Grigorov added a comment -

          Done.
          From now on a cloned instance of the returned DateFormat will be used.

          Show
          Martin Grigorov added a comment - Done. From now on a cloned instance of the returned DateFormat will be used.
          Hide
          Emond Papegaaij added a comment -

          The quickstart attached to this ticket is invalid. SimpleDateFormat.toString is not an indication of the actual instance of SimpleDateFormat because the hashCode method is overridden to return the hash code of the pattern, which will always be the same. If you change the debug output to System.identityHashCode( dateFormat ), you'll see a new instance is used on every call. Therefore, I opt to undo the changed made for this ticket, because they are not needed. DateFormat.getInstance will not return the same instance twice.

          Show
          Emond Papegaaij added a comment - The quickstart attached to this ticket is invalid. SimpleDateFormat.toString is not an indication of the actual instance of SimpleDateFormat because the hashCode method is overridden to return the hash code of the pattern, which will always be the same. If you change the debug output to System.identityHashCode( dateFormat ), you'll see a new instance is used on every call. Therefore, I opt to undo the changed made for this ticket, because they are not needed. DateFormat.getInstance will not return the same instance twice.
          Hide
          SoulSpirit added a comment -

          You're right Emond, I've been tricked by both DateFormat#toString() and the misleading method's name DateFormat#getDateInstance() ( #newDateInstance() seems more appropriate).
          Thanks to everyone and sorry for the misguiding bug submission.

          Show
          SoulSpirit added a comment - You're right Emond, I've been tricked by both DateFormat#toString() and the misleading method's name DateFormat#getDateInstance() ( #newDateInstance() seems more appropriate). Thanks to everyone and sorry for the misguiding bug submission.
          Hide
          Martin Grigorov added a comment -

          The change is reverted in 6.x branch.

          Show
          Martin Grigorov added a comment - The change is reverted in 6.x branch.

            People

            • Assignee:
              Martin Grigorov
              Reporter:
              SoulSpirit
            • Votes:
              0 Vote for this issue
              Watchers:
              4 Start watching this issue

              Dates

              • Created:
                Updated:
                Resolved:

                Development