discomarathon / google-gson

Automatically exported from code.google.com/p/google-gson
0 stars 0 forks source link

Thread issues with date formatter #162

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
What steps will reproduce the problem?
1.  Run high levels of threads which do date serialization
2.  DefaultDateTypeAdapter uses SimpleDateFormat statically
3.  See lots of exceptions on random occasions

What is the expected output? What do you see instead?

Expect date serialization to work!
 public JsonElement serialize(Date src, Type typeOfSrc,
JsonSerializationContext context) {
      String dateFormatAsString = format.format(src);
      return new JsonPrimitive(dateFormatAsString);
    }
Changing the constructor to:
 public DefaultDateTypeAdapter(final String datePattern) {
      this.format = new ThreadLocal<DateFormat>() {
        protected DateFormat initialValue() {
                   new SimpleDateFormat(datePattern);
                };
    }

  public JsonElement serialize(Date src, Type typeOfSrc,
JsonSerializationContext context) {
      String dateFormatAsString = format.get().format(src);
      return new JsonPrimitive(dateFormatAsString);
    }

  public Date deserialize(JsonElement json, Type typeOfT,
JsonDeserializationContext context)
        throws JsonParseException {
      if (!(json instanceof JsonPrimitive)) {
        throw new JsonParseException("The date should be a string value");
      }

      try {
        return format.get().parse(json.getAsString());
      } catch (ParseException e) {
        throw new JsonParseException(e);
      }
    }

Would be a simple fix.

What version of the product are you using? On what operating system?

Latest GSON release.  Issue is using the date formatter statically, as
SimpleDateFormat isn't thread safe, so you'll get random results with the
date format.  It will also randomly throw exceptions.  See stack trace below.

Please provide any additional information below.

java.lang.ArrayIndexOutOfBoundsException: -28   at
sun.util.calendar.BaseCalendar.getCalendarDateFromFixedDate(BaseCalendar.java:43
6)
at
java.util.GregorianCalendar.computeFields(GregorianCalendar.java:2081)  at
java.util.GregorianCalendar.computeFields(GregorianCalendar.java:1996)  at
java.util.Calendar.setTimeInMillis(Calendar.java:1066)  at
java.util.Calendar.setTime(Calendar.java:1032)  at
java.text.SimpleDateFormat.format(SimpleDateFormat.java:785)    at
java.text.SimpleDateFormat.format(SimpleDateFormat.java:778)    at
java.text.DateFormat.format(DateFormat.java:314)    at
com.google.gson.DefaultTypeAdapters$DefaultDateTypeAdapter.serialize(DefaultType
Adapters.java:254)
at 

Original issue reported on code.google.com by mcinto...@gmail.com on 25 Sep 2009 at 4:12

GoogleCodeExporter commented 9 years ago
Good catch. We chose to instead just synchronize the serialize and deserialize 
methods. See r452

Original comment by inder123 on 25 Sep 2009 at 5:15

GoogleCodeExporter commented 9 years ago

Original comment by inder123 on 25 Sep 2009 at 5:15

GoogleCodeExporter commented 9 years ago
Note, the synchronization will fix the threading issues, but there is a 
performance
cost of that versus thread local variables.  In high performance/highly threaded
environments, using SimpleDateFormat has a cost.  Because of the way it's
implemented, with regular expressions, when you're running a lot of threads 
there is
definitely a potential for blocking that could be problematic.  I don't have 
any hard
and fast numbers at the moment, but it might be worth while to run some 
profiling
with at least 16 threads to make sure there aren't any performance issues.

Original comment by mcinto...@gmail.com on 28 Sep 2009 at 5:05

GoogleCodeExporter commented 9 years ago
Yes, we thought about the performance issues regarding synchronization. 
However, the synchronization is 
specific to the default DateTypeAdapter so the impact will be if you have lots 
of dates getting serialized at the 
same time. That shouldn't be a problem in real world situations in my opinion. 
If you have a situation where this 
is a concern, please reopen the bug and provide details and we will take 
another look.

Original comment by inder123 on 28 Sep 2009 at 3:11