Details

    • Type: Bug Bug
    • Status: Resolved
    • Priority: Blocker Blocker
    • Resolution: Fixed
    • Affects Version/s: 4.1.1
    • Fix Version/s: 4.1.2
    • Component/s: Framework, tapestry-core
    • Labels:
      None

      Description

      AssetService declares this field:

      static final DateFormat CACHED_FORMAT = new SimpleDateFormat("EEE, d MMM yyyy HH:mm:ss Z", Locale.ENGLISH);

      However, DateFormat objects are explicitly NOT thread safe. We've encountered this in our in-house concurrency testing. We'll have to patch our local 4.1.1 to fix this before deploying our application.

      a quick search did not find any other references to SimpleDateFormat that look like they could have the same issue.

      A simple CommonsPool object pool of date format objects would fix the problem.

        Activity

        Hide
        Greg Woolsey added a comment -

        Here's what I did in our app, in an alternate asset service implementation class. I just copied AssetService and changed the static DateFormat to a static final ObjectPool from Commons-Pool. Specifically, I used a StackObjectPool, as it is simple and thread-safe:

        private static final ObjectPool CACHED_FORMAT_POOL = new StackObjectPool(new PoolableDateFormatFactory(CACHED_FORMAT, CACHED_FORMAT_LOCALE));

        the PoolableObjectFactory used is just a simple class extending BasePoolableObjectFactory and taking the SimpleDateFormat constructor parameters as it's constructor parameters. It then returns new SimpleDateFormat instances as requested in makeObject().

        The changes to AssetService.cachedResource() are only in the IF block for the modified-since header:

        if (header != null) {
        DateFormat format = null;
        try

        { format = (DateFormat) CACHED_FORMAT_POOL.borrowObject(); modify = format.parse(header).getTime(); }

        catch (ParseException e)

        { e.printStackTrace(); // original catch block }

        catch (Exception e)

        { throw new RuntimeException("error getting pooled date format", e); }

        finally {
        if (format != null) {
        try

        { CACHED_FORMAT_POOL.returnObject(format); }

        catch (Exception e)

        { // ignore - no tear-down needed for returned objects, and the pool will just create a new one if it needs to. }

        }
        }
        }

        Show
        Greg Woolsey added a comment - Here's what I did in our app, in an alternate asset service implementation class. I just copied AssetService and changed the static DateFormat to a static final ObjectPool from Commons-Pool. Specifically, I used a StackObjectPool, as it is simple and thread-safe: private static final ObjectPool CACHED_FORMAT_POOL = new StackObjectPool(new PoolableDateFormatFactory(CACHED_FORMAT, CACHED_FORMAT_LOCALE)); the PoolableObjectFactory used is just a simple class extending BasePoolableObjectFactory and taking the SimpleDateFormat constructor parameters as it's constructor parameters. It then returns new SimpleDateFormat instances as requested in makeObject(). The changes to AssetService.cachedResource() are only in the IF block for the modified-since header: if (header != null) { DateFormat format = null; try { format = (DateFormat) CACHED_FORMAT_POOL.borrowObject(); modify = format.parse(header).getTime(); } catch (ParseException e) { e.printStackTrace(); // original catch block } catch (Exception e) { throw new RuntimeException("error getting pooled date format", e); } finally { if (format != null) { try { CACHED_FORMAT_POOL.returnObject(format); } catch (Exception e) { // ignore - no tear-down needed for returned objects, and the pool will just create a new one if it needs to. } } } }
        Hide
        Massimo Lusetti added a comment -

        Kudos for this catch, i think i came accross this once or twice without recognizing it, thanks!

        Show
        Massimo Lusetti added a comment - Kudos for this catch, i think i came accross this once or twice without recognizing it, thanks!
        Hide
        Jesse Kuhnert added a comment -

        Thanks for the issue / steps to resolve. Pretty much did what you said verbatim. (as it worked out on the other regexp issue)

        Show
        Jesse Kuhnert added a comment - Thanks for the issue / steps to resolve. Pretty much did what you said verbatim. (as it worked out on the other regexp issue)

          People

          • Assignee:
            Jesse Kuhnert
            Reporter:
            Greg Woolsey
          • Votes:
            2 Vote for this issue
            Watchers:
            1 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development